-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add validation for directions.txt #559
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions which I think will streamline this update.
src/main/java/com/conveyal/datatools/manager/gtfsplus/GtfsPlusValidation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/gtfsplus/GtfsPlusValidation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/gtfsplus/GtfsPlusValidation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/gtfsplus/GtfsPlusValidation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/gtfsplus/GtfsPlusValidation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/gtfsplus/GtfsPlusValidation.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. There is an issue with CI failing to resolve before merging. Perhaps the recent dev updates need to be merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, we need a PR against the mtc-deploy
branch first (Keep this PR for dev
after the MTC one is approved).
|
||
Set<String> gtfsRoutes = new HashSet<>(); | ||
if (tableIsDirections) { | ||
// Copy the gtfs routes into a map we can "check them off" in (remove them). Stream is required in order to copy keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stream is required....
Is that sentence still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 5529162
// Expect issues to be zero. | ||
assertThat("Issues count for clean BART feed is zero", validation.issues.size(), equalTo(0)); | ||
// Expect issues to be only one with directions.txt file | ||
assertThat("Issues count for clean BART feed is zero", validation.issues.size(), equalTo(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the description of the test (e.g. "Clean BART feed and incomplete directions.txt results in one issue.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in e9cd115
Updated this to match the latest language in the MTC branch. Should be the same as that previously closed PR. Note if you're testing this you should merge dev into this branch to get the latest updates |
Checklist
dev
before they can be merged tomaster
)Description
It has recently been pointed out that a lack of route records in the GTFS+ directions.txt can cause direction data to become unavailable in downstream systems. This PR provides validation of the GTFS+ directions.txt table to ensure that every route has a corresponding entry.