feat: introduce CurrentDateTime by lionel-nj · Pull Request #855 · MobilityData/gtfs-validator

lionel-nj · 2021-04-15T17:14:52Z

Summary:

This PR introduces CurrentDateTime as a wrapper around ZonedDateTime to be used (in the future) in ValidationContext

Expected behavior:

No change in validation rule logic: same tests should pass.

Please make sure these boxes are checked before submitting your pull request - thanks!

add unit tests for CurrentDateTime
Run the unit tests with gradle test to make sure you didn't break anything
Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
Linked all relevant issues
~~[ ] Include screenshot(s) showing how this pull request works and fixes the issue(s)~~

core/src/main/java/org/mobilitydata/gtfsvalidator/input/CurrentDateTime.java

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java

aababilov

Nice start! Do you mind to merge my #850 first?

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java

core/src/main/java/org/mobilitydata/gtfsvalidator/input/CurrentDateTime.java

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java

core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsTableLoader.java

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/parsing/RowParser.java # core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidationContext.java # core/src/test/java/org/mobilitydata/gtfsvalidator/parsing/RowParserTest.java # main/src/main/java/org/mobilitydata/gtfsvalidator/cli/Main.java # main/src/test/java/org/mobilitydata/gtfsvalidator/table/GtfsLevelTableLoaderTest.java # main/src/test/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidatorTest.java # processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/TableLoaderGenerator.java

- reintroduce ValidationContext - encapsulate ZonedDateTime under CurrentDateTime - update unit tests - update javadocs

lionel-nj · 2021-04-22T20:05:12Z

Thanks @aababilov, modified this PR as suggested.

aababilov

LGTM! Please run

 google-java-format -i $(find . -name '*.java')

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidationContext.java

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java

processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/TableLoaderGenerator.java

…alidator into dependency-injection

aababilov

LGTM! Please merge and I will update my PR.

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidator.java

aababilov · 2021-04-22T20:58:56Z

Lionel, that will crash validator :) You have not modified dependency injection mechanism. Do you mind to leave that to me?

aababilov · 2021-04-22T20:59:15Z

Do you mind to merge #862 first?

lionel-nj · 2021-04-22T21:02:16Z

Lionel, that will crash validator :) You have not modified dependency injection mechanism. Do you mind to leave that to me?

I knew I might I forgotten something. Yes, let's do that! I'll take note for the future 📝

aababilov · 2021-04-22T21:05:54Z

Here is a stacktrace for you. You need a feed with feed_info.txt to observe that.

SEVERE: Runtime exception when loading feed_info.txt
java.lang.IllegalArgumentException: argument type mismatch
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
        at org.mobilitydata.gtfsvalidator.validator.ValidatorLoader.createValidator(ValidatorLoader.java:236)
        at org.mobilitydata.gtfsvalidator.validator.ValidatorLoader.createSingleEntityValidators(ValidatorLoader.java:156)
        at org.mobilitydata.gtfsvalidator.table.GtfsFeedInfoTableLoader.load(GtfsFeedInfoTableLoader.java:95)
        at org.mobilitydata.gtfsvalidator.table.GtfsFeedLoader.lambda$loadAndValidate$0(GtfsFeedLoader.java:113)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at java.base/java.lang.Thread.run(Thread.java:829)

aababilov · 2021-04-22T21:09:04Z

May I suggest the following:

Please submit CurrentDateTime.java , CurrentDateTimeTest.java and ValidationContext.java in this PR. I will write the next PR to make changes to dependency injection and modify the FeedExpirationDateValidator.

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidationContext.java

lionel-nj · 2021-04-22T21:11:53Z

Here is a stacktrace for you. You need a feed with feed_info.txt to observe that.

Thank you very much, I ran it on a dataset that was not using feed_info.txt.

Please submit CurrentDateTime.java , CurrentDateTimeTest.java and ValidationContext.java in this PR.

On it!

aababilov · 2021-04-22T21:18:17Z

Thank you very much, I ran it on a dataset that was not using feed_info.txt.

I will add checks to ValidatorLoader for checking constructors for validator classes even if they are not instantiated. You see, right now if you don't have the feed_info.txt file, then the validators for it are not instantiated and you don't observe the failure.

aababilov · 2021-04-22T21:20:32Z

After you change the code in this PR, you can rename it to "Introduce CurrentDateTime".

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidationContext.java

lionel-nj · 2021-04-22T22:02:40Z

Following your suggestion I only kept changes related to CurrentDateTime.java, CurrentDateTimeTest.java and ValidationContext.java - which unfortunately breaks the execution of GtfsLevelTableLoaderTest. Because of this I added the @Nullable to allow to temporary skip CurrentDateTime in ValidationContext builder.

aababilov

LGTM! Please merge it ;)

lionel-nj · 2021-04-22T22:13:18Z

Thanks @aababilov!

aababilov · 2021-04-22T23:21:13Z

Here you go: #866

refactor: inject CurrentDateTime and GtfsFeedName

79d9f09

lionel-nj mentioned this pull request Apr 15, 2021

feat!: deprecate -f #851

Merged

3 tasks

lionel-nj self-assigned this Apr 15, 2021

lionel-nj changed the title ~~refactor: inject CurrentDateTime and GtfsFeedName~~ [WIP] refactor: inject CurrentDateTime and GtfsFeedName Apr 15, 2021

lionel-nj changed the title ~~[WIP] refactor: inject CurrentDateTime and GtfsFeedName~~ refactor: [WIP] inject CurrentDateTime and GtfsFeedName Apr 15, 2021

lionel-nj changed the title ~~refactor: [WIP] inject CurrentDateTime and GtfsFeedName~~ refactor: [WIP] inject CurrentDateTime and CountryCode Apr 15, 2021

lionel-nj changed the title ~~refactor: [WIP] inject CurrentDateTime and CountryCode~~ refactor: [WIP] Inject CurrentDateTime and CountryCode Apr 15, 2021

lionel-nj commented Apr 15, 2021

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/input/CurrentDateTime.java Outdated Show resolved Hide resolved

lionel-nj commented Apr 15, 2021

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java Outdated Show resolved Hide resolved

lionel-nj mentioned this pull request Apr 15, 2021

Bug: Erroneous open issue count octokit/octokit.js#2073

Closed

7 tasks

aababilov requested changes Apr 16, 2021

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsTableLoader.java Outdated Show resolved Hide resolved

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java Outdated Show resolved Hide resolved

aababilov mentioned this pull request Apr 22, 2021

feat: Dependency injection with ValidatorProvider #862

Merged

lionel-nj added 2 commits April 22, 2021 15:20

apply suggestions from code review

812b6d2

- reintroduce ValidationContext - encapsulate ZonedDateTime under CurrentDateTime - update unit tests - update javadocs

lionel-nj added 2 commits April 22, 2021 16:12

additional unit test

e3f6105

update copyright header

0dca5a4

lionel-nj requested a review from aababilov April 22, 2021 20:13

lionel-nj changed the title ~~refactor: [WIP] Inject CurrentDateTime and CountryCode~~ refactor: inject CurrentDateTime and CountryCode Apr 22, 2021

aababilov requested changes Apr 22, 2021

View reviewed changes

lionel-nj and others added 6 commits April 22, 2021 16:34

style: reformat

6ae4c46

fix: javadocs

5a31de0

ci: reformat code on this branch

1e3ff43

chore: Google Java Format

79f8f6a

remove changes to github action

5b4ec0d

Merge branch 'dependency-injection' of github.com:MobilityData/gtfs-v…

d073f53

…alidator into dependency-injection

lionel-nj requested a review from aababilov April 22, 2021 20:39

aababilov approved these changes Apr 22, 2021

View reviewed changes

aababilov requested changes Apr 22, 2021

View reviewed changes

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidator.java Outdated Show resolved Hide resolved

lionel-nj requested a review from aababilov April 22, 2021 20:56

aababilov reviewed Apr 22, 2021

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidationContext.java Outdated Show resolved Hide resolved

lionel-nj added 2 commits April 22, 2021 17:12

fix: javadocs

508ee3d

Merge branch 'master' into dependency-injection

06be8f7

lionel-nj and others added 5 commits April 22, 2021 17:41

revert changes

b35c5bc

remove unused import

3763c9c

remove file

c2fbd50

reimport file

5f111cc

format changes

53d641d

aababilov requested changes Apr 22, 2021

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidationContext.java Outdated Show resolved Hide resolved

leverage Nullable

c24f7f1

lionel-nj changed the title ~~refactor: inject CurrentDateTime and CountryCode~~ Introduce: CurrentDateTime Apr 22, 2021

remove changes from ValidationContext

24c7934

lionel-nj changed the title ~~Introduce: CurrentDateTime~~ feat: introduce CurrentDateTime Apr 22, 2021

aababilov approved these changes Apr 22, 2021

View reviewed changes

lionel-nj mentioned this pull request Apr 22, 2021

Dependency injection: deprecate ValidationContext #853

Closed

lionel-nj marked this pull request as ready for review April 22, 2021 22:12

lionel-nj merged commit d10efdc into master Apr 22, 2021

lionel-nj deleted the dependency-injection branch April 22, 2021 22:13

Conversation

lionel-nj commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aababilov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lionel-nj commented Apr 22, 2021

Uh oh!

aababilov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aababilov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aababilov commented Apr 22, 2021

Uh oh!

aababilov commented Apr 22, 2021

Uh oh!

lionel-nj commented Apr 22, 2021

Uh oh!

aababilov commented Apr 22, 2021

Uh oh!

aababilov commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lionel-nj commented Apr 22, 2021

Uh oh!

aababilov commented Apr 22, 2021

Uh oh!

aababilov commented Apr 22, 2021

Uh oh!

Uh oh!

lionel-nj commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aababilov left a comment

Choose a reason for hiding this comment

Uh oh!

lionel-nj commented Apr 22, 2021

Uh oh!

aababilov commented Apr 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lionel-nj commented Apr 15, 2021 •

edited

Loading

aababilov commented Apr 22, 2021 •

edited

Loading

lionel-nj commented Apr 22, 2021 •

edited

Loading