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.

Sign up for GitHub

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

Jump to bottom

fix 878: require agency_id when more than one agency, warn when only one #1318

Merged
merged 11 commits into from
Mar 6, 2023
Merged

fix 878: require agency_id when more than one agency, warn when only one #1318

merged 11 commits into from
Mar 6, 2023

Conversation

briandonahue
Copy link
Collaborator

@briandonahue briandonahue commented Jan 24, 2023 •
edited

Summary:

Closes #878

  1. Added test for existing validation requiring agency_id if more than one agency in agency.txt
  2. generate a warning if there is a single agency in agency.txt but agency_id is not provided
  3. generate a warning if agency_id is missing in routes.txt and a single agency is defined in agency.txt with no agency_id
  4. generate an warning if agency_id is missing in routes.txt but is included for single agency in agency.txt

Expected behavior:

N/A

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) (N/A)

@briandonahue briandonahue marked this pull request as draft January 24, 2023 21:56
@isabelle-dr isabelle-dr linked an issue Jan 25, 2023 that may be closed by this pull request
Best Practice: Include agency_id in routes.txt (WARNING) #878
Closed
@isabelle-dr
Copy link
Contributor

See my latest comment here in case that helps :)

@isabelle-dr isabelle-dr mentioned this pull request Feb 6, 2023
Closed
4 tasks
@briandonahue briandonahue mentioned this pull request Feb 8, 2023
Closed
@briandonahue briandonahue marked this pull request as ready for review February 8, 2023 19:57
@briandonahue
Copy link
Collaborator Author

@isabelle-dr I think this is ready for review. I've also commented on #1315 where I think there may be some redundancy with the agency_id check for fare_attributes.txt. This PR should address the conditional required & recommended notices as opposed to just the required notice.

@isabelle-dr
Copy link
Contributor

Sounds good, I will have a look shortly :)

@briandonahue briandonahue changed the title fix: require agency_id when more than one agency, warn when only one fix 878: require agency_id when more than one agency, warn when only one Feb 23, 2023
@briandonahue briandonahue mentioned this pull request Feb 23, 2023
Closed
4 tasks
isabelle-dr
isabelle-dr approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@isabelle-dr isabelle-dr left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @briandonahue for working on this!

@fredericsimard fredericsimard merged commit 2ff1980 into MobilityData:master Mar 6, 2023
@briandonahue briandonahue deleted the issue/878/agency_id_notices branch March 6, 2023 21:37
ryon pushed a commit to JarvusInnovations/gtfs-validator that referenced this pull request Apr 1, 2023
@briandonahue @KClough @ryon
fix 878: require agency_id when more than one agency, warn when only …
6748868 
…one (MobilityData#1318)

* fix: require agency_id when more than one agency, warn when only one

* Remove custom error, use MissingRecommendedField

* Add agency_id validation for fare_attributes.txt, make consistent with routes.txt validator

* formatting

* Update references for missing_recommended_field warning

* Account for possibility of 0 agencies

* add no notice test cases for routes.txt

* inline method

* add no notice test cases for fare_attributes.txt

---------

Co-authored-by: Kevin Clough <kevinclough@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdferris-v2 bdferris-v2 bdferris-v2 approved these changes

@isabelle-dr isabelle-dr isabelle-dr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Best Practice: Include agency_id in routes.txt (WARNING)
5 participants
@briandonahue @isabelle-dr @bdferris-v2 @fredericsimard @KClough