-
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 267: route.long_name should not contain route.short_name #1325
feat 267: route.long_name should not contain route.short_name #1325
Conversation
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/RouteNameValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/RouteNameValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/RouteNameValidator.java
Outdated
Show resolved
Hide resolved
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.
Thank you for working on this @briandonahue !
Your new validator notice replaces https://github.com/MobilityData/gtfs-validator/blob/master/RULES.md#route_short_and_long_name_equal. Can you remove it in this PR, so that we can close #267?
We also need the update in RULES.md :)
@isabelle-dr changing this to a draft to avoid accidental merging - realizing we could use a few more test cases and document good/bad data examples |
@bdferris-v2 Any thoughts on using parameterized tests where they make sense? For this where there are many ways the I see this library which looks useful, but wondering if you had any other recommendations or concerns with doing this. |
@isabelle-dr @bdferris-v2 I've made some significant changes. Documenting the rule made me realize it was missing some use cases and that it may not be perfectly clear and/or covering all potential issues. So far, I've updated to account for short/long are equal (case insensitive), long cannot start with short and be followed by a space, or disallowed character Here are my current test cases for bad data:
Please let me know if any should be removed or added. |
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.
I don't know enough of this codebase to approve this PR. The code looks clean, I've added some comments/suggestions. Let me know if I can help further.
main/src/test/java/org/mobilitydata/gtfsvalidator/validator/RouteNameValidatorTest.java
Outdated
Show resolved
Hide resolved
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.
A few comments in-line, thank you so much for working on this @briandonahue!
A great addition to this validator :)
Better examples! Co-authored-by: isabelle-dr <[email protected]>
main/src/test/java/org/mobilitydata/gtfsvalidator/validator/RouteNameValidatorTest.java
Outdated
Show resolved
Hide resolved
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.
Modulo the small changes requested, this LGTM.
@bdferris-v2 Made those fixes, thanks. |
…://github.com/MobilityData/gtfs-validator into issue/267/route_long_name_contains_short_name
One last small change before we merge :) |
@isabelle-dr because the acceptance test workflow only has permission to post the comment with the report summary if the PR is coming from a branch of the |
That's a good call. Definitely the bad list test case is a great example for parameterizing a test. We can have this "feature" for free if we decide to upgrade the JUnit version. JUnit 5 Parameterized Tests |
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.
LGTM!
@isabelle-dr @bdferris-v2 This was created before I had access to the main repo. Should I close this PR and open a new one from a local branch? |
Two things. The acceptance test still ran and you can access the logs in the ✅ Rule acceptance tests passed. That said, per #1349, the acceptance test only tracks changes to |
@briandonahue good to merge? |
@isabelle-dr Yes! 😄 |
…tyData#1325) * fix: 267 - route.long_name should not contain route.short_name * formatting * Remove unnecessary comment * Remove unescaped regex, do startsWith check then regex on remainder * update RULES.md * update RULES.md examples * Add more test examples and remove redundant warning * remove redundant test data * fix typo * Remove redundant rule * 'should not', not 'can not' * simplify test data * Missed a spot to remove redundant rule * Update RULES.md Better examples! Co-authored-by: isabelle-dr <[email protected]> * Missed a spot to remove redundant rule * Make requested updates to rules docs * Address PR feedback * Correct field name --------- Co-authored-by: Kevin Clough <[email protected]> Co-authored-by: isabelle-dr <[email protected]>
Summary:
Closes #267. Based on implementation suggestion by @aababilov here
Expected behavior:
Should generate WARNING level notice that
route.long_name
should not containroute.short_name
as referenced in Best Practices: https://gtfs.org/schedule/best-practices/#routestxtPlease make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything