feat: Add MixedCase validation (#881) by briandonahue · Pull Request #1340 · MobilityData/gtfs-validator

briandonahue · 2023-02-16T17:50:46Z

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!

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

github-actions · 2023-02-16T18:42:40Z

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

github-actions · 2023-02-16T20:39:24Z

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

github-actions · 2023-02-16T22:42:38Z

❌ Invalid acceptance test.
New Errors: 1350 out of 1391 datasets (~97%) are invalid due to code change, which is above the provided threshold of 1%.
Dropped Errors: 0 out of 1391 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1391 sources (~0 %) are corrupted.
Commit: 6b1c96f
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

github-actions · 2023-02-17T16:59:02Z

❌ Invalid acceptance test.
New Errors: 1350 out of 1391 datasets (~97%) are invalid due to code change, which is above the provided threshold of 1%.
Dropped Errors: 0 out of 1391 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1391 sources (~0 %) are corrupted.
Commit: e71e102
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

github-actions · 2023-02-17T21:29:36Z

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: ./gradlew goJF.

github-actions · 2023-02-17T21:36:45Z

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: ./gradlew goJF.

...ssor/src/main/java/org/mobilitydata/gtfsvalidator/processor/MixedCaseValidatorGenerator.java

core/src/main/java/org/mobilitydata/gtfsvalidator/testing/LoadingHelper.java

github-actions · 2023-02-17T22:53:04Z

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

core/src/main/java/org/mobilitydata/gtfsvalidator/notice/MixedCaseNotice.java

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

bdferris-v2 · 2023-02-20T15:39:18Z

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

  String agencyId();

  @Required
+  @MixedCase


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.

bdferris-v2 · 2023-02-20T15:50:37Z

...ssor/tests/src/main/java/org/mobilitydata/gtfsvalidator/processor/tests/MixedCaseSchema.java

+public interface MixedCaseSchema {
+  @Required
+  @MixedCase
+  String mixedCase();


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.

I also found it a little confusing that "MixedCase" is both used as the annotation and the entity class names in the tests below.

@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.

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

github-actions · 2023-02-20T21:15:35Z

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

github-actions · 2023-02-21T17:00:56Z

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

isabelle-dr · 2023-03-06T20:39:14Z

We generally name the notices with words that describe the problem (equal_shape_distance_diff_coordinates, invalid_currency, missing_required_file).
I wonder if we could find another name for this one that would follow the same pattern. Maybe non_mixed_case_field?

briandonahue · 2023-03-06T21:22:53Z

I wonder if we could find another name for this one that would follow the same pattern. Maybe non_mixed_case_field?

@isabelle-dr How about mixed_case_required or mixed_case_recommended since it's a warning / best practice?

isabelle-dr · 2023-03-06T23:12:30Z

I thinkmixed_case_recommended_field would be good :)

briandonahue · 2023-03-07T21:22:59Z

I thinkmixed_case_recommended_field would be good :)

@isabelle-dr done!

github-actions · 2023-03-07T22:13:29Z

✅ 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: 0 out of 1419 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1419 sources (~0 %) are corrupted.
Commit: d65ac75
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

isabelle-dr · 2023-03-08T15:26:19Z

@briandonahue one last comment before we merge, based on Brian's comment here and after reading the Best Practice.
I think we need to explain why this is recommended in a little mode in detail.

Can we replace the long description of this notice with the following:

This field contains customer-facing text and should use Mixed Case (upper and lower case letters) to ensure good readability when displayed to riders. Avoid the use of abbreviations throughout the feed (e.g. St. for Street) unless a location is called by its abbreviated name (e.g. “JFK Airport”). Abbreviations may be problematic for accessibility by screen reader software and voice user interfaces.

Lastly, can you remove the extra "." character in the short description (in the table)?

After this, we are good to go!

briandonahue · 2023-03-08T16:33:36Z

@isabelle-dr updated!

github-actions · 2023-03-08T17:27:25Z

✅ 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: 0 out of 1419 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1419 sources (~0 %) are corrupted.
Commit: efa3726
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

isabelle-dr

LGTM 🚀

RULES.md

isabelle-dr · 2023-03-07T23:31:19Z

RULES.md

+| `filename`    	| Name of the faulty file.              	| String 	|
+| `fieldName`    	| Name of the faulty field.              	| String 	|
+
+#### Affected files & fields


💯 for specifying the fields here

isabelle-dr · 2023-03-08T19:22:03Z

It looks like we have a problem with test pack doc 🤔

bdferris-v2 · 2023-03-08T19:53:23Z

@isabelle-dr it was a transient failure. I re-ran the failing test and it's now run cleanly.

isabelle-dr · 2023-03-08T22:01:04Z

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

Add MixedCase annotation

654e6e6

Add to column descriptor

95744ff

Add validator generator

724517a

Remove 'Required' for warning notice

b7b3f34

Added test, column name is wrong...

4b8bf7a

set as warning

0550a24

briandonahue added 2 commits February 17, 2023 16:39

Add MixedCase annotation to all necessary fields

f10495b

convert field name to snake case

35dfbc0

briandonahue commented Feb 17, 2023

View reviewed changes

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

briandonahue commented Feb 17, 2023

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/testing/LoadingHelper.java Show resolved Hide resolved

bdferris-v2 reviewed Feb 20, 2023

View reviewed changes

core/src/main/java/org/mobilitydata/gtfsvalidator/notice/MixedCaseNotice.java Outdated Show resolved Hide resolved

bdferris-v2 reviewed Feb 20, 2023

View reviewed changes

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

bdferris-v2 reviewed Feb 20, 2023

View reviewed changes

.../tests/src/test/java/org/mobilitydata/gtfsvalidator/processor/tests/MixedCaseSchemaTest.java Outdated Show resolved Hide resolved

briandonahue added 4 commits February 20, 2023 14:10

Use field name converter

9132742

readability suggestions

ea79176

Rename MixedCase in tests to MixedCaseTest

6c63277

Merge branch 'master' into issue/881/mixed-case

29b025e

briandonahue added 2 commits February 21, 2023 11:02

Add rule definition

2303080

Rename notice to be clearer

0467c71

briandonahue marked this pull request as ready for review February 21, 2023 16:05

bdferris-v2 approved these changes Feb 22, 2023

View reviewed changes

briandonahue changed the title ~~Fix 881: Add MixedCase validation~~ feat: Add MixedCase validation (#881) Mar 6, 2023

Change notice name

989eacf

briandonahue added 2 commits March 8, 2023 11:32

Update long description, correct punctuation

5fac37c

Merge branch 'master' into issue/881/mixed-case

e053eae

isabelle-dr approved these changes Mar 8, 2023

View reviewed changes

isabelle-dr merged commit 345bb4c into master Mar 8, 2023

isabelle-dr deleted the issue/881/mixed-case branch March 8, 2023 22:01

isabelle-dr mentioned this pull request Apr 19, 2023

Mixed case validation notice schema is incomplete #1392

Closed

Conversation

briandonahue commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 16, 2023

Uh oh!

github-actions bot commented Feb 16, 2023

Uh oh!

github-actions bot commented Feb 16, 2023

Uh oh!

github-actions bot commented Feb 17, 2023

Uh oh!

github-actions bot commented Feb 17, 2023

Uh oh!

github-actions bot commented Feb 17, 2023

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 17, 2023

Uh oh!

Uh oh!

Uh oh!

bdferris-v2 Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

bdferris-v2 Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

bdferris-v2 Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

briandonahue Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

bdferris-v2 Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Feb 20, 2023

Uh oh!

github-actions bot commented Feb 21, 2023

Uh oh!

isabelle-dr commented Mar 6, 2023

Uh oh!

briandonahue commented Mar 6, 2023

Uh oh!

isabelle-dr commented Mar 6, 2023

Uh oh!

briandonahue commented Mar 7, 2023

Uh oh!

github-actions bot commented Mar 7, 2023

Uh oh!

isabelle-dr commented Mar 8, 2023

Uh oh!

briandonahue commented Mar 8, 2023

Uh oh!

github-actions bot commented Mar 8, 2023

Uh oh!

isabelle-dr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isabelle-dr Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

isabelle-dr commented Mar 8, 2023

Uh oh!

bdferris-v2 commented Mar 8, 2023

Uh oh!

isabelle-dr commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

briandonahue commented Feb 16, 2023 •

edited

Loading