feat!: deprecate -f by lionel-nj · Pull Request #851 · MobilityData/gtfs-validator

lionel-nj · 2021-04-14T15:12:01Z

closes #539

Summary:

This PR provides support to make -f an optional argument when running the validator. This is required to run the validator programatically for:

integration to the MobilityDatabase
integration tests purpose

Expected behavior:

if -f is now optional. If provided, phone numbers will be validated according to the country code, else phone number validation is skipped.

No NPEs should be thrown.

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

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)~~

- handle possible NPEs - extend unit tests

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

barbeau

I support making -f optional but I'm not convinced that wrapping null within GtfsFeedName is the right way to approach this. Seems dangerous and prone to future NPEs.

Instead, I think a better approach would be to have a new -c command line parameter for country code and truly make -f optional, and deprecate it in future releases (unless we see other value in keeping it). If either are provided you can validate phone number, and if neither are provided you skip that rule.

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

- implement -c - implement new class CountryCode - adjust existing code to use this instead of GtfsFeedName - add unit tests - make phone number validation conditional

- adjust country code definition in tests - up small caps in tests

aababilov · 2021-04-14T23:45:25Z

Thanks for working in that direction!

One more thing came to my mind. Let's remove the monolithic ValidationContext and inject only direct dependencies.

"Inject only direct dependencies" is one of the main DI principles at Google. Here is what it means.

This approach is not recommended:

public class ShowBudgets {
   private final Account account;

   @Inject
   ShowBudgets(Customer customer) {
     account = customer.getPurchasingAccount();
   }

This one is preferred:

public class ShowBudgets {
   private final Account account;

   @Inject
   ShowBudgets(Account account) {
     this.account = account;
   }

So, instead of injecting ValidationContext, we should inject the CountryCode instead.
We probably should not inject ZonedDateTime but we need a wrapper class for it (say, CurrentDateTime) because ZonedDateTime does not have a semantic that it is the current datetime.

This change will also simplify our unit tests since they will not have to create the full ValidationContext which may grow in the future.

How does that sound?

lionel-nj · 2021-04-15T13:43:13Z

How does that sound?

Good to me! 👍🏾
Opened #853 to this extent.

Edit: This is implemented in #855

barbeau

Thanks @lionel-nj! Some comments in-line. The README and other docs also need to be updated.

main/src/main/java/org/mobilitydata/gtfsvalidator/cli/Arguments.java

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

core/src/main/java/org/mobilitydata/gtfsvalidator/parsing/RowParser.java

- implement field getter - remove asUnvalidatedString to simplify code - update unit tests

barbeau

Thanks @lionel-nj, some thoughts in-line.

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

README.md

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

barbeau · 2021-04-15T15:24:22Z

Oh, if this is a breaking change, the commit/PR title should include a ! - https://www.conventionalcommits.org/en/v1.0.0/

TODO: remove mocks

lionel-nj · 2021-04-15T17:05:46Z

Removing mocks from CliParametersAnalyzerTest in #854

aababilov

Nice, can't wait to see it merged!

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

core/src/main/java/org/mobilitydata/gtfsvalidator/parsing/RowParser.java

README.md

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/parsing/RowParser.java # core/src/test/java/org/mobilitydata/gtfsvalidator/parsing/RowParserTest.java # processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/TableLoaderGenerator.java

- use special code ZZ for unknown or invalid territories - adapt unit tests - override toString - remove support for null value

lionel-nj · 2021-04-20T15:46:17Z

Thanks @aababilov for reviewing! I provided the changes you suggested in the previous commits. At the exception of this one: #851 (comment)

private static final String ZZ = "ZZ";

Since we do not have support for null, I made this constant public, so that we can access from the main method in order to avoid NPE.

main/src/test/java/org/mobilitydata/gtfsvalidator/cli/ArgumentsTest.java

aababilov · 2021-04-20T20:32:39Z

Beautiful! Please format the code and submit it, I can't wait to see it in upstream!

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

lionel-nj · 2021-04-20T20:40:13Z

Thanks @aababilov for this feedback!

barbeau

Nice work @lionel-nj! 👍

feat: make -f optional

7fb3cac

- handle possible NPEs - extend unit tests

lionel-nj self-assigned this Apr 14, 2021

lionel-nj requested a review from barbeau April 14, 2021 15:12

lionel-nj commented Apr 14, 2021

View reviewed changes

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

test: add previous unit tests

0e385be

lionel-nj changed the title ~~feat: make -f an optional cLI argument~~ feat: make -f an optional CLI argument Apr 14, 2021

lionel-nj requested a review from maximearmstrong April 14, 2021 15:19

lionel-nj added this to the v2.1 milestone Apr 14, 2021

lionel-nj mentioned this pull request Apr 14, 2021

feat: acceptance tests #848

Merged

4 tasks

barbeau reviewed Apr 14, 2021

View reviewed changes

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

lionel-nj added 2 commits April 14, 2021 17:14

feat: deprecate -f

2f3fba4

- implement -c - implement new class CountryCode - adjust existing code to use this instead of GtfsFeedName - add unit tests - make phone number validation conditional

fix: tests

d41a8aa

- adjust country code definition in tests - up small caps in tests

lionel-nj requested a review from barbeau April 14, 2021 21:18

lionel-nj changed the title ~~feat: make -f an optional CLI argument~~ feat: deprecate -f Apr 14, 2021

barbeau reviewed Apr 15, 2021

View reviewed changes

lionel-nj mentioned this pull request Apr 15, 2021

Dependency injection: deprecate ValidationContext #853

Closed

lionel-nj added 3 commits April 15, 2021 10:27

Leverage Optional

3626354

- implement field getter - remove asUnvalidatedString to simplify code - update unit tests

Merge remote-tracking branch 'origin/master' into optional-feed-name

87297ee

docs: update .md files to match latest development

47f962d

lionel-nj requested a review from barbeau April 15, 2021 14:33

barbeau reviewed Apr 15, 2021

View reviewed changes

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

README.md Show resolved Hide resolved

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

lionel-nj added 2 commits April 15, 2021 12:13

apply suggestions from code review

789b717

Add deprecation message when -f is used as CLI arg

251cb31

lionel-nj changed the title ~~feat: deprecate -f~~ feat!: deprecate -f Apr 15, 2021

fix: tests

c5a0314

TODO: remove mocks

lionel-nj requested a review from barbeau April 15, 2021 17:05

This was referenced Apr 15, 2021

feat: introduce CurrentDateTime #855

Merged

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

Closed

aababilov requested changes Apr 16, 2021

View reviewed changes

lionel-nj mentioned this pull request Apr 20, 2021

Parsing failed message, but no details in report.json #858

Closed

lionel-nj added 7 commits April 20, 2021 10:23

remove formatting changes

a2e55cf

merge changes from masters

2897a08

apply suggestions from code review

f87308d

- use special code ZZ for unknown or invalid territories - adapt unit tests - override toString - remove support for null value

update docs

a799a4c

additional unit test

7b1634f

update docs

2c5dd7a

lionel-nj requested a review from aababilov April 20, 2021 15:46

aababilov reviewed Apr 20, 2021

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/cli/ArgumentsTest.java Outdated Show resolved Hide resolved

style: format code

0f88a5e

style: format code

c618514

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

barbeau approved these changes Apr 20, 2021

View reviewed changes

lionel-nj merged commit d7fa7d4 into master Apr 20, 2021

lionel-nj deleted the optional-feed-name branch April 20, 2021 20:48

isabelle-dr modified the milestones: v2.1, v3.0.0 Oct 28, 2021

isabelle-dr mentioned this pull request Oct 28, 2021

3.0.0 Release tracking #1046

Closed

4 tasks

This was referenced Nov 10, 2021

False positives for InvalidPhoneNumberNotice #1058

Closed

fix: Skip phone number validation when country code is unknown #1062

Merged

Conversation

lionel-nj commented Apr 14, 2021

Uh oh!

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barbeau commented Apr 15, 2021

Uh oh!

lionel-nj commented Apr 15, 2021

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!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

aababilov commented Apr 20, 2021

Uh oh!

lionel-nj commented Apr 20, 2021

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aababilov commented Apr 14, 2021 •

edited

Loading

lionel-nj commented Apr 15, 2021 •

edited

Loading

lionel-nj commented Apr 20, 2021 •

edited

Loading