Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Move gtfsvalidator.cli package into new cli sub-module. #1229

Merged
merged 4 commits into from
Aug 9, 2022

Conversation

bdferris-v2
Copy link
Collaborator

Summary:

Per discussion in #1188, I'm proposing to move the gtfsvalidator.cli package and associated classes for the GTFS Validator command-line interface into their own Gradle sub-module. This will support future modularization and packaging of the validator in ways that don't assume a CLI.

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

@bdferris-v2 bdferris-v2 marked this pull request as ready for review August 7, 2022 12:11
Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot @bdferris-v2

Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot @bdferris-v2

Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested the behavior locally, and it seems that the JAR file works, but there is a problem with the validators, even though all the status checks passed.

This is stdout when using the JAR file built on the master branch:
Capture d’écran, le 2022-08-08 à 18 21 05
Capture d’écran, le 2022-08-08 à 18 22 24

and now the stdout with the new JAR file:
Capture d’écran, le 2022-08-08 à 18 20 44

Also, comparing the reports indicates a problem. Using master:
Capture d’écran, le 2022-08-08 à 18 32 52

using the new one:
Capture d’écran, le 2022-08-08 à 18 32 24

One problem I see here is that our acceptance tests don't detect this kind of problem. They verify that new rules don't add too many new errors, not if errors are disappearing. Perhaps we should change them to check other anomalies. I initially approved this PR because all tests were passing, so we should definitely find a way to capture problems like these. cc @isabelle-dr

@bdferris-v2
Copy link
Collaborator Author

@maximearmstrong indeed that's a subtle bug. When shadowJar minimization moved from main to cli, it caused the dynamically-loaded validators in main to be dropped since shadowJar is minimizing dependencies (which main now is). I added an exclude to fix this.

Regarding acceptance tests, maybe we can add an issue to track fixing that as well?

@maximearmstrong
Copy link
Contributor

All good. I tested it locally and it worked well. Thanks for this modification @bdferris-v2

Regarding acceptance tests, I've opened #1232. I think we could add a few new rules. Your thoughts are welcome :)

@maximearmstrong maximearmstrong merged commit 242246a into MobilityData:master Aug 9, 2022
bdferris-v2 added a commit that referenced this pull request Aug 15, 2022
Cleanup work on issue #1188. When we submitted #1229, I had a todo to update `acceptance_test.yml` to reference the cli/ module on the master branch once submitted. It's now been submitted, so here is the update to resolve the TODO.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move gtfsvalidator.cli package into a separate cli Gradle sub-project
2 participants