feat: Add validation support for transfers.txt transfer_type 4 and 5.
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!
- [x] Run the unit tests with
gradle testto make sure you didn't break anything - [x] 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)
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_idORto_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_idORto_stop_iddefined] AND [loc_typeof 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.
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?
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