Fix 1198: separate required column from required value by briandonahue · Pull Request #1344 · MobilityData/gtfs-validator

briandonahue · 2023-02-28T22:44:01Z

Summary:

Closes #1198. Added a new annotation @RequiredColumn to allow for scenario where a column must exist in the file, but values for that column may be empty.

It may be useful in the future to separate the existing @Required annotation into @RequiredFile and @RequiredValue annotations. I've opened #1343 for discussion of that topic.

Expected behavior:

Given the following transfers.txt file, the following should not throw an error:

from_stop_id,to_stop_id,transfer_type,min_transfer_time
1,2,,

This is a valid use case according to the spec in that the transfer_type value is empty which indicates a "Recommended transfer point between routes".

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
(N/A) Include screenshot(s) showing how this pull request works and fixes the issue(s)

briandonahue · 2023-02-28T22:46:23Z

@bdferris-v2 As this uses code generation, I was unsure if/how we could add tests for this. Please let me know if you have thoughts.

github-actions · 2023-02-28T23:39:01Z

✅ Rule acceptance tests passed.
New Errors: 0 out of 1419 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 9 out of 1419 datasets (~1%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1419 sources (~0 %) are corrupted.
Commit: ae74379
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

core/src/main/java/org/mobilitydata/gtfsvalidator/annotation/RequiredColumn.java

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/DefaultTableHeaderValidator.java

bdferris-v2 · 2023-03-01T00:27:42Z

Regarding testing, could you introduce a test case in processor/tests with a test schema that exercises the @RequiredColumn behavior?

briandonahue · 2023-03-01T17:02:51Z

@bdferris-v2 I added tests for both @Required and @RequiredColumn annotations to differentiate. I did not test the @Required annotation when it is placed at the Class/file level, as I wasn't sure that was in scope and also since I opened #1343 as a possible change to using the same annotation for those two purposes.

github-actions · 2023-03-01T18:31:47Z

✅ Rule acceptance tests passed.
New Errors: 0 out of 1419 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 9 out of 1419 datasets (~1%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1419 sources (~0 %) are corrupted.
Commit: 5d7a6a3
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

core/src/main/java/org/mobilitydata/gtfsvalidator/annotation/Required.java

github-actions · 2023-03-06T18:29:02Z

✅ Rule acceptance tests passed.
New Errors: 1 out of 1419 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 9 out of 1419 datasets (~1%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1419 sources (~0 %) are corrupted.
Commit: 21fc4e1
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

isabelle-dr · 2023-03-06T22:28:02Z

Just a side thought: We have a missing_required_column in this validator, and it is triggered when the whole column is missing (as opposed to a missing required field in a given row).
I don't think it necessarily interacts with the work in this PR, just something to keep in mind.

Merging this PR, thank you @briandonahue!

github-actions · 2023-03-06T23:45:36Z

✅ Rule acceptance tests passed.
New Errors: 1 out of 1419 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 9 out of 1419 datasets (~1%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1419 sources (~0 %) are corrupted.
Commit: 739c52f
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

isabelle-dr · 2023-03-08T22:26:45Z

@briandonahue could you resolve the conflicts? :)

briandonahue · 2023-03-09T19:50:47Z

@briandonahue could you resolve the conflicts? :)

@isabelle-dr done!

github-actions · 2023-03-09T20:47:01Z

✅ Rule acceptance tests passed.
New Errors: 1 out of 1419 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 9 out of 1419 datasets (~1%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1419 sources (~0 %) are corrupted.
Commit: 6ab0f91
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

isabelle-dr · 2023-03-10T16:11:31Z

Thank you for this awesome contribution @briandonahue! ✨

…1344) * feat: separate required column from required value. * Use better example in annotation docs * rename parameters back to *Columns * Adding test for RequiredColumn and Required Annotations * punctuation --------- Co-authored-by: isabelle-dr <[email protected]>

feat: separate required column from required value.

501fd5b

briandonahue requested a review from bdferris-v2 February 28, 2023 22:44

bdferris-v2 reviewed Mar 1, 2023

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/annotation/RequiredColumn.java Outdated Show resolved Hide resolved

bdferris-v2 reviewed Mar 1, 2023

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/validator/DefaultTableHeaderValidator.java Outdated Show resolved Hide resolved

briandonahue added 2 commits March 1, 2023 09:51

Use better example in annotation docs

694e514

rename parameters back to *Columns

1afd8a0

briandonahue force-pushed the issue/1198/required_field_transfer_type branch from 0c37195 to 7b3310d Compare March 1, 2023 16:59

Adding test for RequiredColumn and Required Annotations

4d8c2fb

briandonahue force-pushed the issue/1198/required_field_transfer_type branch from 7b3310d to 4d8c2fb Compare March 1, 2023 17:00

bdferris-v2 reviewed Mar 6, 2023

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/annotation/Required.java Outdated Show resolved Hide resolved

bdferris-v2 approved these changes Mar 6, 2023

View reviewed changes

punctuation

640a7cd

isabelle-dr approved these changes Mar 6, 2023

View reviewed changes

Merge branch 'master' into issue/1198/required_field_transfer_type

6a1faf0

Merge branch 'master' into issue/1198/required_field_transfer_type

999789a

isabelle-dr merged commit 9bde7a1 into master Mar 10, 2023

isabelle-dr deleted the issue/1198/required_field_transfer_type branch March 10, 2023 16:11

isabelle-dr mentioned this pull request Mar 10, 2023

Create a @RecommendedColumn annotation #1350

Closed

briandonahue mentioned this pull request May 31, 2023

feat: separate and clarify file, column, value annotations #1468

Closed

4 tasks

Conversation

briandonahue commented Feb 28, 2023

Uh oh!

briandonahue commented Feb 28, 2023

Uh oh!

github-actions bot commented Feb 28, 2023

Uh oh!

Uh oh!

Uh oh!

bdferris-v2 commented Mar 1, 2023

Uh oh!

briandonahue commented Mar 1, 2023

Uh oh!

github-actions bot commented Mar 1, 2023

Uh oh!

Uh oh!

github-actions bot commented Mar 6, 2023

Uh oh!

isabelle-dr commented Mar 6, 2023

Uh oh!

github-actions bot commented Mar 6, 2023

Uh oh!

isabelle-dr commented Mar 8, 2023

Uh oh!

briandonahue commented Mar 9, 2023

Uh oh!

github-actions bot commented Mar 9, 2023

Uh oh!

isabelle-dr commented Mar 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants