-
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: Add MixedCase validation (#881) #1340
Conversation
✅ Rule acceptance tests passed. |
✅ Rule acceptance tests passed. |
❌ Invalid acceptance test. |
❌ Invalid acceptance test. |
This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: |
This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: |
...ssor/src/main/java/org/mobilitydata/gtfsvalidator/processor/MixedCaseValidatorGenerator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/mobilitydata/gtfsvalidator/testing/LoadingHelper.java
Show resolved
Hide resolved
✅ Rule acceptance tests passed. |
core/src/main/java/org/mobilitydata/gtfsvalidator/notice/MixedCaseNotice.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/mobilitydata/gtfsvalidator/annotation/MixedCase.java
Outdated
Show resolved
Hide resolved
@@ -34,6 +29,7 @@ public interface GtfsAgencySchema extends GtfsEntity { | |||
String agencyId(); | |||
|
|||
@Required | |||
@MixedCase |
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 know this annotation is intended for more than this field, but it's going to be interesting to see how this actually validates for agency name. For example, if the agency name is "SFMTA" or "MARTA" (aka an acronym) it won't actually be mixed-case.
public interface MixedCaseSchema { | ||
@Required | ||
@MixedCase | ||
String mixedCase(); |
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 would actually name this field something different. Like "someField". I think having the field named "mixedCase" causes just a bit of confusion in the unit-test about when "mixedCase" is being used as field name or in some other context.
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 also found it a little confusing that "MixedCase" is both used as the annotation and the entity class names in the tests below.
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 I modeled the CurrencyAmount
example, so it has the same issue. I renamed it to MixedCaseTestSchema
, though not sure it's a major improvement in readability. Let me know what you think.
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.
Ha, it's true. Maybe it was less noticeable in that test because I inlined all the file and field names in the test, as opposed to referencing CurrencyAmount.FILENAME, etc? Sorry for the conflicting advice.
.../tests/src/test/java/org/mobilitydata/gtfsvalidator/processor/tests/MixedCaseSchemaTest.java
Outdated
Show resolved
Hide resolved
✅ Rule acceptance tests passed. |
✅ Rule acceptance tests passed. |
We generally name the notices with words that describe the problem (equal_shape_distance_diff_coordinates, invalid_currency, missing_required_file). |
@isabelle-dr How about |
I think |
@isabelle-dr done! |
✅ Rule acceptance tests passed. |
@briandonahue one last comment before we merge, based on Brian's comment here and after reading the Best Practice. Can we replace the long description of this notice with the following:
Lastly, can you remove the extra "." character in the short description (in the table)? After this, we are good to go! |
@isabelle-dr updated! |
✅ 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.
LGTM 🚀
| `filename` | Name of the faulty file. | String | | ||
| `fieldName` | Name of the faulty field. | String | | ||
|
||
#### Affected files & fields |
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.
💯 for specifying the fields here
It looks like we have a problem with test pack doc 🤔 |
@isabelle-dr it was a transient failure. I re-ran the failing test and it's now run cleanly. |
Ok, merging now :) |
* Add MixedCase annotation * Add to column descriptor * Add validator generator * Remove 'Required' for warning notice * Added test, column name is wrong... * set as warning * Add MixedCase annotation to all necessary fields * convert field name to snake case * Use field name converter * readability suggestions * Rename MixedCase in tests to MixedCaseTest * Add rule definition * Rename notice to be clearer * Change notice name * Update long description, correct punctuation
Summary:
Closes #881
Expected behavior:
Explain and/or show screenshots for how you expect the pull request to work in your testing (in case other devices exhibit different behavior).
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything