-
Notifications
You must be signed in to change notification settings - Fork 100
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: Initial support for GTFS Fares v2 - fare media #1305
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.
@bdferris-v2 Thanks for working on this 🙏! Can we make this fork available for stakeholders producing this data?
Here are the name changes that we've made after you created this PR:
- fare_payment_types.txt -> fare_payment_options.txt
- fare_payment_type_group_id -> fare_payment_option_group_id
- fare_payment_type_name -> fare_payment_option_name
- fare_payment_type -> fare_payment_option_type
@isabelle-dr done |
main/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsFarePaymentOptionTypeEnum.java
Outdated
Show resolved
Hide resolved
✅ Rule acceptance tests passed. |
✅ Rule acceptance tests passed. |
@bdferris-v2 this is now unblocked :) I suggest we keep the |
@isabelle-dr I had already updated the bulk of references - just had to fix a few lingering mentions in the docs. This is ready for review. |
✅ Rule acceptance tests passed. |
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.
@bdferris-v2 there seems to be a problem with the structure of the duplicate_fare_media
notice. See the image of the report.
Here is the JSON if it helps:
{"code":"duplicate_fare_media",
"severity":"WARNING",
"totalNotices":1,
"sampleNotices":[{
"fareMedia1":{
"csvRowNumber":6.0,"fareMediaId":"cash"},
"fareMedia2":{
"csvRowNumber":7.0,"fareMediaId":"cash2"}}]}
Co-authored-by: isabelle-dr <[email protected]>
Co-authored-by: isabelle-dr <[email protected]>
✅ Rule acceptance tests passed. |
So back to the high-level question from @isabelle-dr. I'll admit I took a slightly different approach to We have lots of notices where we are documenting the differences between two entities in a file. The existing convention for notices has been to introduce fields like:
and corresponding:
While that works, it leads to duplication of field descriptions/documentation and logic for setting field values. Here, I took a different approach, where I bundled the fields for an entity together ( The resulting JSON schema looks like:
I'll admit that renders a little weirdly in the existing report UI, but I have a follow-up PR that would make it render like this: Note the nested rows of field name headers at the top. Thoughts? |
✅ Rule acceptance tests passed. |
Okay, I understand. This seems more straightforward indeed. I am wondering if the users that programmatically consume the JSON report will have issues with this. |
As noted on Slack, @atvaccaro will review for Cal-ITP impacts; thanks for the call-out @isabelle-dr, this does affect us! |
Cross-posting from Slack: would it be possible to stick with either totally unnested or totally nested notices? Right we just parse each record as flat i.e. no nesting (https://github.com/cal-itp/data-infra/blob/main/airflow/dags/create_external_tables/gtfs_schedule_v2/validation_notices.yml#L41-L299) and while I wouldn’t be opposed to the nesting in general, my understanding is that the PR would only change to nesting for this specific fares work. (please correct me if I’m wrong) We can handle nested fields for sure, just wondering about overall consistency. |
It's true that this change is only for the single fare-related notice. There are a handful of existing notices that I believe could benefit from a nested field approach:
but I acknowledge it's a more significant decision to migrate all of them and probably one we couldn't make in time for the next release. And if we were going to make a breaking change to the notice schema, I suppose I'd want to use it as an opportunity to fix some other inconsistencies as well ( So I guess that's a long way of saying that I will revert the nest field notices in this PR for now. |
✅ Rule acceptance tests passed. |
While I don't like that we're adding a different format/schema just for our use case here, I agree that breaking backwards compatibility (by changing all other notices' formats to work this way too) should be done in a more coordinated fashion. Si I'm okay with the format. 👍 |
I should note that I do not parse GTFS Validator's output in production yet. I have built a quick prototype script once, and I'm planning to use it in production, but not yet. 😊 |
Thank you for the feedback. |
✅ Rule acceptance tests passed. |
Sounds good to me! We'd definitely be on board with normalization and consistency fixes, but agree that doing it in a single breaking change would be ideal. |
* Initial entry for fare payment type schema and validation. * Update to reflect new fare_payment_options.txt naming. * Update to reflect fare payment option group discussion. * Update to match fare_media.txt proposal. * fare medium => fare media * Clean up a few last usages of `medium`. * Documentation. * Fix link in RULES.md Co-authored-by: isabelle-dr <[email protected]> * Fix link in RULES.md Co-authored-by: isabelle-dr <[email protected]> * Remove extraneous tick-tick-ticks. * Flatten notice fields. * Add new notice fields. * Update RULES.md --------- Co-authored-by: isabelle-dr <[email protected]>
* Initial entry for fare payment type schema and validation. * Update to reflect new fare_payment_options.txt naming. * Update to reflect fare payment option group discussion. * Update to match fare_media.txt proposal. * fare medium => fare media * Clean up a few last usages of `medium`. * Documentation. * Fix link in RULES.md Co-authored-by: isabelle-dr <[email protected]> * Fix link in RULES.md Co-authored-by: isabelle-dr <[email protected]> * Remove extraneous tick-tick-ticks. * Flatten notice fields. * Add new notice fields. * Update RULES.md --------- Co-authored-by: isabelle-dr <[email protected]>
As described in google/transit#355. This is a WIP that is intended to track the proposed modifications the spec. Closes #1301.
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything