docs: Define severity for each notice by lionel-nj · Pull Request #726 · MobilityData/gtfs-validator

lionel-nj · 2021-02-05T16:45:15Z

closes #709
closes #472
closes #417

Summary:

This PR suggests a category for each specific notice.

Expected behavior:

Tests should pass.

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-" (PR title must follow the conventional commit specification)
Linked all relevant issues
~~[ ] Include screenshot(s) showing how this pull request works and fixes the issue(s)~~

lionel-nj · 2021-02-05T16:46:55Z

@barbeau
I did this first categorization of notices, what do you think about that?

Also:

🚩Tests are broken for now. They will be fixed once we agree on the category of each notice.

aababilov · 2021-02-08T02:57:10Z

That would increase the amount of job for me. I see that you set severity of DuplicateKeyError and DuplicatedColumnNotice to INFO while they are ERRORs. That will lead to incorrect validation reports and to errors in the core code. Please let me set the priorities because I know what they should be and what we actually have in the production feeds in the world.

barbeau

Thanks @lionel-nj! My comments on errors vs. warnings are in-line, following the approach of the canonical validator where only items that are explicitly required in the spec (based on RFC 2119 language definitions) can be errors. I believe some require the feedback of @MobilityData/transit-specs.

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

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

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

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

...ain/java/org/mobilitydata/gtfsvalidator/notice/BlockTripsWithOverlappingStopTimesNotice.java

...rc/main/java/org/mobilitydata/gtfsvalidator/notice/SameNameAndDescriptionForRouteNotice.java

main/src/main/java/org/mobilitydata/gtfsvalidator/notice/StartAndEndDateOutOfOrderNotice.java

main/src/main/java/org/mobilitydata/gtfsvalidator/notice/StartAndEndTimeOutOfOrderNotice.java

.../mobilitydata/gtfsvalidator/notice/StopTimeWithArrivalBeforePreviousDepartureTimeNotice.java

...java/org/mobilitydata/gtfsvalidator/notice/StopTimeWithDepartureBeforeArrivalTimeNotice.java

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NonAsciiOrNonPrintableCharNotice.java # main/src/main/java/org/mobilitydata/gtfsvalidator/notice/InconsistentAgencyTimezoneNotice.java

lionel-nj · 2021-02-11T20:06:53Z

Thanks @barbeau just updated this PR. I will wait for @MobilityData/transit-specs to confirm the comments I left unresolved.

- change several notice severity level

# Conflicts: # main/src/main/java/org/mobilitydata/gtfsvalidator/notice/OverlappingFrequencyNotice.java

lionel-nj · 2021-02-17T08:16:10Z

@barbeau this PR is now ready for review. Thanks @timMillet !

barbeau

@lionel-nj This looks pretty good! I found two things that needs to be changed, plus another I'd like feedback on from @MobilityData/transit-specs.

main/src/main/java/org/mobilitydata/gtfsvalidator/notice/InconsistentAgencyTimezoneNotice.java

main/src/main/java/org/mobilitydata/gtfsvalidator/notice/StartAndEndDateOutOfOrderNotice.java

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

…idator into categorize-notices

barbeau · 2021-02-18T22:54:39Z

@lionel-nj Could you resolve the conflicts here, and change DuplicatedColumnNotice to ERROR?

If you do that, and update the notices merged in #756 (just the Javadocs, I think, to match those in this PR), then I think this should be ready to merge!

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/notice/FieldParsingError.java

aababilov · 2021-02-18T23:42:35Z

That is a big contribution, Lionel. Thank you!

I left some comments mostly based on the real production feeds.

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

aababilov · 2021-02-18T23:30:26Z

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

+/**
+ * A CSV file is empty.
+ *
+ * <p>Severity: {@code SeverityLevel.ERROR}


We do not enforce that check at Google, so this should be rather a warning. This is not a blocker for using feed data if, e.g., there is an empty file pathways.txt. An empty file is practically equivalent to a missing file.

We have 3 situations that may be treated as equivalent:

a file is missing

a file has only headers

a file is empty

GTFS marks certain files as required. I think that we should amend the GTFS reference and say that a required file must contain at least one record (not counting the headers).

The Javadocs for CsvFile.isEmpty() currently say:

Tells if the file is empty, i.e. it has no rows and even no headers.

And GTFS says:

The first line of each file must contain field names.

...so this seems to align as an ERROR to me. Let's discuss in a new issue/PR if needed so we can get this merged - it's getting hard to manage this PR due to length.

aababilov · 2021-02-18T23:32:25Z

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

+/**
+ * An enum has an unexpected value.
+ *
+ * <p>Severity: {@code SeverityLevel.WARNING}


We treat it as an ERROR at Google but it probably may be a WARNING in the upstream.

@barbeau suggested to make that one a WARNING

I think this has to be a WARNING? There are a lot of examples where producers have extended enumeration values in their own feeds with values that don't exist in the spec. And to my knowledge the spec doesn't explicitly say you can't use non-standard enum values.

from #726 (comment)

...ain/java/org/mobilitydata/gtfsvalidator/notice/BlockTripsWithOverlappingStopTimesNotice.java

main/src/main/java/org/mobilitydata/gtfsvalidator/notice/FeedExpirationDateNotice.java

aababilov · 2021-02-18T23:34:57Z

...main/java/org/mobilitydata/gtfsvalidator/notice/FeedInfoLangAndAgencyLangMismatchNotice.java

+/**
+ * {@code agency.agency_lang} and {@code feed_info.feed_lang} do not match
+ *
+ * <p>Severity: {@code SeverityLevel.WARNING}


Let's make it an ERROR. If feed gives different agency_lang and feed_lang, the intended meaning of that is undefined. Google rejects such feeds.

Let's discuss in a new issue/PR if needed so we can get this merged - it's getting hard to manage this PR due to length.

New issue on this matter: #762

...rc/main/java/org/mobilitydata/gtfsvalidator/notice/SameNameAndDescriptionForRouteNotice.java

aababilov · 2021-02-18T23:40:37Z

...rc/main/java/org/mobilitydata/gtfsvalidator/notice/SameNameAndDescriptionForRouteNotice.java

+ * `routes.route_long_name`/`routes/route_short_name.
+ * <p>"Do not simply duplicate the name of the location." (http://gtfs.org/reference/static#routestxt)
+ *
+ * <p>Severity: {@code SeverityLevel.ERROR}


This should be a WARNING. This does not make the feed non-functional.

I think we can safely call this an ERROR. The spec says do not.

from #726 (comment)

aababilov · 2021-02-25T22:16:16Z

Hi @lionel-nj

Please can you remove FieldParsingError.java file that resurrected in this PR?

Thanks!

lionel-nj · 2021-02-25T22:42:34Z

Sure @aababilov! It's done in #775

chore: identify each type of notice

e4a489c

lionel-nj self-assigned this Feb 5, 2021

lionel-nj mentioned this pull request Feb 5, 2021

chore: categorize each type of notice #725

Closed

3 tasks

lionel-nj requested a review from barbeau February 5, 2021 16:46

This was linked to issues Feb 5, 2021

Error 55 (language) should be changed from ERROR to WARNING #472

Closed

Errors vs warnings - Incorrect rule about shapes.txt #417

Closed

This was unlinked from issues Feb 8, 2021

Errors vs warnings - Incorrect rule about shapes.txt #417

Closed

Error 55 (language) should be changed from ERROR to WARNING #472

Closed

barbeau reviewed Feb 11, 2021

View reviewed changes

Merge branch 'master' into categorize-notices

9beef97

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NonAsciiOrNonPrintableCharNotice.java # main/src/main/java/org/mobilitydata/gtfsvalidator/notice/InconsistentAgencyTimezoneNotice.java

lionel-nj added 4 commits February 11, 2021 16:09

Apply suggestions from code review

433718e

- change several notice severity level

Apply suggestions from code review

f2d56c5

- change several notice severity level

Merge branch 'master' into categorize-notices

d60e579

# Conflicts: # main/src/main/java/org/mobilitydata/gtfsvalidator/notice/OverlappingFrequencyNotice.java

rework notice types following spec team input

14e5310

lionel-nj marked this pull request as ready for review February 17, 2021 08:15

lionel-nj requested review from aababilov and barbeau February 17, 2021 08:16

fix typo in javadocs

d355fbd

This was referenced Feb 17, 2021

Error 55 (language) should be changed from ERROR to WARNING #472

Closed

Errors vs warnings - Incorrect rule about shapes.txt #417

Closed

rework notice type

a5155d5

barbeau changed the title ~~chore: identify each type of notice~~ docs: identify each type of notice Feb 17, 2021

Fix typo

c1cd002

barbeau reviewed Feb 17, 2021

View reviewed changes

lionel-nj added 2 commits February 18, 2021 13:24

rework notice types to match specification

fc6ca43

Merge branch 'categorize-notices' of github.com:MobilityData/gtfs-val…

6b007a7

…idator into categorize-notices

barbeau mentioned this pull request Feb 18, 2021

feat: Split FieldParsingError into different classes #756

Merged

Merge branch 'master' into categorize-notices

67803f5

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/notice/FieldParsingError.java

aababilov requested changes Feb 18, 2021

View reviewed changes

lionel-nj added 2 commits February 19, 2021 09:15

docs: update javadocs

ac337d0

update notice severity

eea17a0

lionel-nj changed the title ~~docs: identify each type of notice~~ chore: identify each type of notice Feb 19, 2021

lionel-nj and others added 4 commits February 19, 2021 09:43

docs: update RULES.md to include severity level definitions

236e43c

docs: update javadocs

12a1760

rework notice severity

2e138ac

Update notice severity definition to be canonical

2f98236

barbeau changed the title ~~chore: identify each type of notice~~ docs: Identify severity for each notice Feb 19, 2021

barbeau changed the title ~~docs: Identify severity for each notice~~ docs: Define severity for each notice Feb 19, 2021

barbeau merged commit b7de88b into master Feb 19, 2021

barbeau deleted the categorize-notices branch February 19, 2021 17:22

lionel-nj mentioned this pull request Feb 19, 2021

Update notice severity definition in code #762

Closed

barbeau mentioned this pull request Feb 19, 2021

docs: Align SeverityLevel Javadocs with RULES.md #764

Merged

4 tasks

lionel-nj mentioned this pull request Feb 26, 2021

refactor: notice constructor #773

Merged

3 tasks

Conversation

lionel-nj commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lionel-nj commented Feb 5, 2021

Uh oh!

aababilov commented Feb 8, 2021

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lionel-nj commented Feb 11, 2021

Uh oh!

lionel-nj commented Feb 17, 2021

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barbeau commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aababilov commented Feb 18, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aababilov commented Feb 25, 2021

Uh oh!

lionel-nj commented Feb 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lionel-nj commented Feb 5, 2021 •

edited

Loading

barbeau commented Feb 18, 2021 •

edited

Loading