feat: Initial support for Fares v2 validation. by bdferris-v2 · Pull Request #1228 · MobilityData/gtfs-validator

bdferris-v2 · 2022-08-05T03:23:08Z

Per issue #1201, adds validation for files and fields added for Fares v2 base spec changes.

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

bdferris-v2 · 2022-08-05T03:24:12Z

Initial draft, mostly to see what happens when validating a few existing feeds.

…t' column.

processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/GtfsAnnotationProcessor.java

isabelle-dr · 2022-08-09T11:15:21Z

main/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsFareTransferRuleSchema.java

+
+  @PrimaryKey(translationRecordIdType = UNSUPPORTED)
+  @Positive
+  int durationLimit();


Just a question: if we have a negative integer in this field, will this trigger the invalid_integer notice?

I can confirm it triggers at number_out_of_range validation error.

bdferris-v2 · 2022-09-14T22:56:20Z

Now that #1248 has been committed and integrated, this PR is ready for official review.

KClough

Not an official reviewer, but lgtm.

isabelle-dr

Thanks, @bdferris-v2 🔥
Two things:

We are missing documentation for invalid_fare_transfer_rule_transfer_count and fare_transfer_rule_missing_transfer_count.
I would like to include three additional rules in the scope of this PR.

missing_fare_rule_file
- spec mention in Dataset files: fare_rules.txt is required if fare_attributes.txt is defined.
- pseudo code: if fare_attributes.txt if present and fare_rules.txt is absent, trigger the notice.
forbidden_fare_rule_file
- spec mention in Dataset files: fare_rules.txt is forbidden if fare_attributes.txt is not defined.
- pseudo code: if fare_attributes.txt if absent and fare_rules.txt is present, trigger the notice.
fare_transfer_rule_forbidden_duration_limit
- spec mention in fare_transfer_rules.duration_limit_type: Forbidden if fare_transfer_rules.duration_limit is empty.
- pseudo code: If [fare_transfer_rules.duration_limit == '' and fare_transfer_rules.duration_limit_type != ''], trigger the notice

I have a copy of the fares v2 spec changes with my comments in https://bit.ly/faresv2-validator.

maximearmstrong · 2022-09-27T23:35:55Z

fare_transfer_rule_forbidden_duration_limit

spec mention in fare_transfer_rules.duration_limit_type: Forbidden if fare_transfer_rules.duration_limit is empty.

pseudo code: If [fare_transfer_rules.duration_limit == '' and fare_transfer_rules.duration_limit_type != ''], trigger the notice

@isabelle-dr I think this has been already added with the FareTransferRuleDurationLimitWithoutTypeNotice here. Could you confirm this is what you were looking for?

maximearmstrong

Great work! I've only one small comment. I think after Isabelle's requested changes are done, we'll be ready to merge. Thanks @bdferris-v2

...in/java/org/mobilitydata/gtfsvalidator/validator/FareTransferRuleTransferCountValidator.java

…s-validator into issue/1201/fares

…idation check.

bdferris-v2 · 2022-09-29T20:05:46Z

I believe @isabelle-dr was correct that I didn't have a validation check for when duration_limit_type was specified but duration_limit was not. I've added that now.

I believe all validation rules have documentation in RULES.md at this point. Let me know if you see things differently.

As for fare_rules.txt and fare_attributes.txt, those seem to be clarifications of the Fares V1 spec. I'm happy to add support for that, but I'd argue for splitting it out into a separate PR to keep things (relatively) focused. Thoughts?

isabelle-dr

Hello! Thanks for making this advance 🙏

For the documentation, I believe we are still missing the first rule you have in FareTransferCountValidator.java (line 47). It is invalid_fare_transfer_rule_transfer_count.

As for the two additional rules I mentioned here, I see fare_rules.txt was introduced in Fares v1, but it went from being optional to conditionally required/forbidden in the PR that added GTFS-Fares v2 base implementation on google/transit (reference).
If the scope of this PR is adding validation for the changes introduced in this PR, I would say it's in scope.

...in/java/org/mobilitydata/gtfsvalidator/validator/FareTransferRuleTransferCountValidator.java

…ing conventions of other notices. Add documentation RULES.md.

isabelle-dr

Looks great!

Initial support for Fares v2 validation.

6e90eaa

bdferris-v2 linked an issue Aug 5, 2022 that may be closed by this pull request

Add rules for the Fares v2 base implementation #1201

Closed

bdferris-v2 added 3 commits August 4, 2022 22:33

Add validation for currency amount.

fe7052c

Add workaround for the very large values present in the areas.txt 'wk…

2e72a0d

…t' column.

Bump areas.txt max-chars-per-column to unlimited.

cd58e3a

owades mentioned this pull request Aug 5, 2022

User Story: Run the Fares v2 validator for each agency cal-itp/data-infra#1664

Closed

bdferris-v2 mentioned this pull request Aug 8, 2022

Add rules for the Fares v2 base implementation #1201

Closed

KClough reviewed Aug 8, 2022

View reviewed changes

processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/GtfsAnnotationProcessor.java Outdated Show resolved Hide resolved

isabelle-dr reviewed Aug 9, 2022

View reviewed changes

bdferris-v2 added 2 commits August 9, 2022 08:38

Remove debug statement.

59862fe

Merge branch 'master' into issue/1201/fares

dfdf243

bdferris-v2 mentioned this pull request Aug 31, 2022

feat: Add @CurrencyAmount annotation for currency amount validation #1248

Merged

3 tasks

bdferris-v2 added 6 commits September 14, 2022 15:13

Merge branch 'master' into issue/1201/fares

9495243

Update to use @CurrencyAmount annotation.

89d1554

Add documentation for new fares notices.

9285ea3

Fix typo.

82107ed

Add documentation for invalid currency amount.

4026388

Fix typo.

da9cf96

bdferris-v2 marked this pull request as ready for review September 14, 2022 22:55

Merge branch 'master' into issue/1201/fares

0691570

KClough approved these changes Sep 21, 2022

View reviewed changes

isabelle-dr requested changes Sep 21, 2022

View reviewed changes

maximearmstrong suggested changes Sep 27, 2022

View reviewed changes

...in/java/org/mobilitydata/gtfsvalidator/validator/FareTransferRuleTransferCountValidator.java Outdated Show resolved Hide resolved

bdferris-v2 added 4 commits September 29, 2022 10:39

Merge branch 'issue/1201/fares' of https://github.com/bdferris-v2/gtf…

4c3f66e

…s-validator into issue/1201/fares

Add documentation for FareTransferRuleDurationLimitWithoutTypeNotice.

594b913

Fix variable name.

a0f69f2

Add fare_transfer_rule_duration_limit_type_without_duration_limit val…

a965192

…idation check.

Remove blank lines.

e8fae01

isabelle-dr reviewed Oct 1, 2022

View reviewed changes

...in/java/org/mobilitydata/gtfsvalidator/validator/FareTransferRuleTransferCountValidator.java Outdated Show resolved Hide resolved

isabelle-dr added this to the Q4 2022 milestone Oct 3, 2022

bdferris-v2 added 3 commits October 3, 2022 11:03

Rename InvalidFareTransferRuleTransferCountNotice to better match nam…

5f12a8c

…ing conventions of other notices. Add documentation RULES.md.

Merge branch 'master' into issue/1201/fares

aa0b7a1

Add missing field to documentation.

aa6e4f8

isabelle-dr approved these changes Oct 3, 2022

View reviewed changes

isabelle-dr merged commit 40c5bc8 into MobilityData:master Oct 3, 2022

isabelle-dr mentioned this pull request Oct 4, 2022

feat: Validation for conditionally required/forbidden fare_rules.txt file. #1261

Closed

3 tasks

This was referenced May 31, 2023

Add back support for @GtfsTable.maxCharsPerColumn #1461

Closed

fix: Add back support for @GtfsTable.maxCharsPerColumn. #1462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Initial support for Fares v2 validation.#1228

feat: Initial support for Fares v2 validation.#1228
isabelle-dr merged 21 commits intoMobilityData:masterfrom
bdferris-v2:issue/1201/fares

bdferris-v2 commented Aug 5, 2022

Uh oh!

bdferris-v2 commented Aug 5, 2022

Uh oh!

Uh oh!

isabelle-dr Aug 9, 2022

Uh oh!

bdferris-v2 Aug 9, 2022

Uh oh!

bdferris-v2 commented Sep 14, 2022

Uh oh!

KClough left a comment

Uh oh!

isabelle-dr left a comment •

edited

Loading

Uh oh!

maximearmstrong commented Sep 27, 2022 •

edited

Loading

Uh oh!

maximearmstrong left a comment

Uh oh!

Uh oh!

bdferris-v2 commented Sep 29, 2022

Uh oh!

isabelle-dr left a comment

Uh oh!

Uh oh!

isabelle-dr left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

bdferris-v2 commented Aug 5, 2022

Uh oh!

bdferris-v2 commented Aug 5, 2022

Uh oh!

Uh oh!

isabelle-dr Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

bdferris-v2 Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

bdferris-v2 commented Sep 14, 2022

Uh oh!

KClough left a comment

Choose a reason for hiding this comment

Uh oh!

isabelle-dr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximearmstrong commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximearmstrong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdferris-v2 commented Sep 29, 2022

Uh oh!

isabelle-dr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isabelle-dr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

isabelle-dr left a comment •

edited

Loading

maximearmstrong commented Sep 27, 2022 •

edited

Loading