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

chore: check stops.zone_id presence #936

Merged
merged 4 commits into from
Jul 26, 2021
Merged

Conversation

lionel-nj
Copy link
Contributor

@lionel-nj lionel-nj commented Jul 23, 2021

closes #237

Summary:

This PR provides support to implement #237

Expected behavior:

New notice:

  • StopWithoutZoneIdNotice (severity level = WARNING - to be upgraded to ERROR in the future).

Per GTFS specification, stops.zone_id should be provided if fare information is provided using fare_rules.txt - if this requirement is not met, a notice should be emitted.

Note that this rule does not apply to records from stops.txt that represent a stop or en entrance (i.e. stops.location_type = 1 or 2) - in this case, no notice should be emitted.

Gtfs specification reference:

| zone_id | ID | Conditionally Required | Identifies the fare zone for a stop. If this record represents a station or station entrance, the zone_id is ignored.

Conditionally Required:
- Required if providing fare information using fare_rules.txt
- Optional otherwise.|

from https://github.com/google/transit/blob/master/gtfs/spec/en/reference.md#stopstxt

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)

@lionel-nj lionel-nj self-assigned this Jul 23, 2021
@lionel-nj lionel-nj added the GTFS Reference Used for Adding or changing rules that belong in the GTFS reference label Jul 23, 2021
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 :)

@lionel-nj lionel-nj merged commit 8e71f46 into master Jul 26, 2021
@lionel-nj lionel-nj deleted the task/chek-zone-id-presence branch July 26, 2021 13:34
@aababilov
Copy link
Collaborator

This Java code was not formatted, so an autoformat commit was submitted automatically. As we discussed before, such commits have a disadvantage - they hide the actual ownership of changed lines.

  1. Please format the Java code before submitting. You can use google-java-format -i $(find . -name '*.java')
  2. Please add an automatic test that a pull request contains properly formatted code. Unformatted code may not be submitted.

@lionel-nj
Copy link
Contributor Author

This Java code was not formatted, so an autoformat commit was submitted automatically. As we discussed before, such commits have a disadvantage - they hide the actual ownership of changed lines.

Indeed, we have a workflow to check said formatting. After verification it used to only execute on push on master.
#953 fixes that: the workflow is executed on pull requests and fails if PR's code is not formatted properly.

}
for (GtfsStop stop : stopTable.getEntities()) {
if (isStationOrEntrance(stop)) {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This return exits the whole validate function and interrupts validation of the rest of the file. Same for the return below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic here has been modified in #972 where validate is as follows:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GTFS Reference Used for Adding or changing rules that belong in the GTFS reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Zone ID presence verification (GTFS Rule)
4 participants