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: Refactor cli.Main into ValidationRunner for easier reuse. #1145

Merged

Conversation

bdferris-v2
Copy link
Collaborator

Per discussion in issue #1135 and https://bit.ly/gtfs-validator-packaged-exe#heading=h.fcbgc2jy8dh, we'd like to refactor code in cli.Main into a new class that's easier to call from other contexts, including the new GUI-based app.

This PR moves the bulk of the code in gtfsvalidator.cli.Main into a new gtfsvalidator.runner.ValidationRunner class that does the work for setting up and running validation. The configuration for the run is passed via a new ValidationRunnerConfig class.

Both gtfsvalidator.cli.Main and gtfsvalidator.app.gui.Main have been refactored to use this new validation runner. The gui app now uses the return code from the runner to potentially open system_errors.json when appropriate.

Command-line argument parsing is left in the existing Arguments class, which I've refactored to produce a config object as its primary output. I'll also merged in the logic from gtfsvalidator/cli/CliParametersAnalyzer.java, since it made the interface for Arguments simpler and I think conceptually it seems reasonable to include it there.

Per discussion in issue MobilityData#1135 and https://bit.ly/gtfs-validator-packaged-exe#heading=h.fcbgc2jy8dh,
we'd like to refactor code in cli.Main into a new class that's easier to call from other contexts,
including the new GUI-based app.
…e ValidationRunner return status. Use that info in the gui app to show the system_errors.json file when appropriate.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

2 small things before approval. Otherwise, this is great. Thank you @bdferris-v2

@bdferris-v2
Copy link
Collaborator Author

I addressed the suggested changes and test are now fully passing. I believe this is ready for a final review.

@maximearmstrong maximearmstrong changed the title Refactor cli.Main into ValidationRunner for easier reuse. feat: Refactor cli.Main into ValidationRunner for easier reuse. May 13, 2022
@maximearmstrong
Copy link
Contributor

We had to reconfigure the CLA wizard, so if possible, could you re-sign it with your second account? We will be ready to merge right after.

@bdferris-v2
Copy link
Collaborator Author

I just resigned. Let me know if you need anything else?

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.

All good! Thank you @bdferris-v2

@maximearmstrong maximearmstrong merged commit 5ad1d78 into MobilityData:master May 13, 2022
Copy link
Member

@barbeau barbeau 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 @bdferris-v2!

*/
public static void printSummary(long startNanos, GtfsFeedContainer feedContainer) {
final long endNanos = System.nanoTime();
if (!feedContainer.isParsedSuccessfully()) {
Copy link
Member

Choose a reason for hiding this comment

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

The Status codes put us in a good position to handle #1096 as well via something like Status.PARSING_FAILED, which is to surface the condition where parsing failed and many validators weren't invoked. That can be in a future PR.

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.

Refactor desktop validation app to call validation code directly instead of cli.Main
5 participants