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: Add validation support for transfers.txt transfer_type 4 and 5. #1266

Merged
merged 6 commits into from
Oct 20, 2022

Conversation

bdferris-v2
Copy link
Collaborator

Closes #1264.

This adds validation support for the newly added transfers.txt transfer_type values of 4 and 5 for in-seat transfers. It additionally adds conditional validation checks for from_stop_id, to_stop_id, from_route_id, and to_route_id, which must not be specified if these new transfer types are used.

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)

@isabelle-dr isabelle-dr added this to the v4.0 milestone Oct 5, 2022
@isabelle-dr
Copy link
Contributor

isabelle-dr commented Oct 6, 2022

Maybe I'm getting something wrong, but after having a check on a test dataset, it seems like forbidden_field appears for every row of transfers.txt that has:

  • [values 4 or 5 in transfer_type] AND [from_stop_id OR to_stop_id defined].

The spec says in transfers.from_stop_id & to_stop_id (reference).

Refering to a station is forbiden for transfer_types 4 and 5.

This notice should be triggered for every row of transfers.txt that has:

  • [values 4 or 5 in transfer_type] AND [from_stop_id OR to_stop_id defined] AND [loc_type of the stop referenced = 1]

I am also seeing that this addition in the spec has 2 typos and doesn't really follow the style for other Conditionally Required fields, maybe I can try to rephrase a bit.

@bdferris-v2
Copy link
Collaborator Author

Hmm... when I read the spec change there, I'll admit I did initially read it as applying to any type of stop. While I can see why a station reference wouldn't make sense here, it's not clear to me why ANY stop reference is needed here. Specifically, for an in-seat transfer between two trips, the stop would always need to be the last stop of the incoming trip and the first stop of the outgoing trip, as anything else wouldn't make sense. And in that context, I thought the spec was just indicating that the stop should be left blank because it can just be inferred from the trip itself.

But if the stop is required, then I would additionally check that it is indeed the last or first stop of the trip, as anything else would be malformed. But mostly, I'm not sure what the intention of the spec addition was here?

@bdferris-v2
Copy link
Collaborator Author

Ok, based off the discussion in the slack channel, I've implemented the following validation logic:

  • the following is flagged as an ERROR based on the spec
    • stop_ids: if the transfer type is 4 or 5, stations must not be used
    • trip_ids: required if the transfer type is 4 or 5
  • the following is flagged as a WARNING so that our users can get the info
    • if the transfer type is 4 or 5, the stops linked must be the first & last of a trip in the documentation for this issue, we explain that if the intention is the use-case is mid-trip in-seat transfers, the warning can be ignored.

As part of this, I've also introduced a TransferDirection helper class that simplified some of the logic of dealing with continual from_X and to_X checks. I updated some existing transfers.txt validation to use that logic as well.

@KClough and @isabelle-dr this is ready for another look

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.

Thank you so much for this contribution @bdferris-v2!
I have ran this PR against a test dataset and it works perfectly ✨.

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.

Add validation for transfers.txt transfer_type=4 and 5
3 participants