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

Add validation for directions.txt #559

Merged
merged 8 commits into from
Oct 23, 2023
Merged

Conversation

philip-cline
Copy link
Contributor

@philip-cline philip-cline commented Sep 26, 2023

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

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.

Copy link
Contributor

@br648 br648 left a 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.

@br648 br648 assigned philip-cline and unassigned br648 Sep 27, 2023
@philip-cline philip-cline assigned br648 and unassigned philip-cline Sep 27, 2023
Copy link
Contributor

@br648 br648 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. There is an issue with CI failing to resolve before merging. Perhaps the recent dev updates need to be merged?

@br648 br648 assigned philip-cline and unassigned br648 Sep 28, 2023
@philip-cline philip-cline changed the base branch from dev to mtc-deploy September 28, 2023 13:58
@philip-cline philip-cline changed the base branch from mtc-deploy to dev September 28, 2023 13:58
Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a 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.
Copy link
Contributor

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?

Copy link
Contributor Author

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));
Copy link
Contributor

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.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in e9cd115

@philip-cline
Copy link
Contributor Author

philip-cline commented Oct 20, 2023

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

@philip-cline philip-cline merged commit 5d426b7 into dev Oct 23, 2023
6 checks passed
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.

3 participants