refactor: distinguish between info, warning, and error notices by lionel-nj · Pull Request #716 · MobilityData/gtfs-validator

lionel-nj · 2021-02-02T19:07:50Z

closes #549

Summary:

This PR is a refactor proposal aimed at implementing the distinction between INFO, WARNING, and ERROR notices.

Expected behavior:

by default: SystemError severity level is set to SeverityLevel.ERROR
by default: ValidationNotice severity level is set to SeverityLevel.ERROR

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)~~

- implement new rule logic - write additional unit tests - update documentation

Co-authored-by: Sean Barbeau <[email protected]>

… into non-ascii-id

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NonAsciiOrNonPrintableCharNotice.java # core/src/test/java/org/mobilitydata/gtfsvalidator/parsing/RowParserTest.java

lionel-nj · 2021-02-02T19:16:00Z

[WIP] Listing of errors/warnings/system errors: #709

aababilov

This is a good start! Thank you, Lionel.

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

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

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

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/RouteNameValidator.java

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

- clarify javadocs - reset Notice to unmutable elements: - remove setter - define 2 constructors for ValidationNotice class - include SeverityLevel in .equals(), .toString() and .hashCode() methods from Notice class

lionel-nj · 2021-02-03T18:55:58Z

Thanks @aababilov for your review! I modified the code and provided sample usage for SeverityLevel.INFO and SeverityLevel.WARNING.

lionel-nj · 2021-02-03T19:06:21Z

I noticed that some notices can be used in different context (as ERROR or WARNING) e.g. MissingTripEdgeNotice which can be an error if stop_times.arrival_time is missing, or a warning if stop_times.departure_times is missing.

An arrival time must be specified for the first and the last stop in a trip.
http://gtfs.org/reference/static#stop_timestxt

Is providing two constructors for this kind of notice the right approach to this situation (as done in 156ccf5)?

aababilov · 2021-02-04T00:38:07Z

156ccf5 is very nice. We actually need just a single constructor that requires to provide severity explicitly, so that the caller never forgets to set it.

aababilov · 2021-02-04T00:42:37Z

Very good, ship it! And thanks!

…lass

- rework notices usage to include SeverityLevel

lionel-nj · 2021-02-04T17:09:02Z

Thanks @aababilov!

barbeau

@lionel-nj Thanks for this, it mostly LGTM! A few corrections to typos and a comment in-line.

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

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

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/MissingTripEdgeValidator.java

Co-authored-by: Sean Barbeau <[email protected]>

…idator into errors-vs-warnings

barbeau

👍

lionel-nj and others added 6 commits February 2, 2021 10:30

feat: non ascii character in id

7318510

- implement new rule logic - write additional unit tests - update documentation

chore: modify notice code

db185dd

apply suggestion fro code review

e06c2d4

Co-authored-by: Sean Barbeau <[email protected]>

fix typo in formatting

a978248

Merge branch 'non-ascii-id' of github.com:MobilityData/gtfs-validator…

035ea5b

… into non-ascii-id

refactor: additional field to distinguish errors from warnings

bf3ac5b

lionel-nj added this to the v2.0 milestone Feb 2, 2021

lionel-nj self-assigned this Feb 2, 2021

Merge branch 'master' into errors-vs-warnings

4305d12

# Conflicts: # core/src/main/java/org/mobilitydata/gtfsvalidator/notice/NonAsciiOrNonPrintableCharNotice.java # core/src/test/java/org/mobilitydata/gtfsvalidator/parsing/RowParserTest.java

lionel-nj changed the title ~~Errors vs warnings~~ refactor: distinguish errors/warning/system errors Feb 2, 2021

lionel-nj requested review from aababilov and barbeau February 2, 2021 19:09

docs: add javadoc for clarity

eb0e7d6

aababilov requested changes Feb 3, 2021

View reviewed changes

lionel-nj added 3 commits February 3, 2021 14:39

Merge branch 'master' into errors-vs-warnings

6ba7197

chore: apply comments from code review

84ff9e0

- clarify javadocs - reset Notice to unmutable elements: - remove setter - define 2 constructors for ValidationNotice class - include SeverityLevel in .equals(), .toString() and .hashCode() methods from Notice class

chore: provide sample usage of SeverityLevel.INFO

737c232

chore: provide sample usage of SeverityLevel.WARNING

156ccf5

aababilov approved these changes Feb 4, 2021

View reviewed changes

lionel-nj changed the title ~~refactor: distinguish errors/warning/system errors~~ refactor: distinguish between info, warning, and error notices Feb 4, 2021

lionel-nj added 2 commits February 4, 2021 12:59

refactor: remove overloading constructor from MissingTripEdgeNotice c…

1695644

…lass

fix: failing tests

361ecc9

- rework notices usage to include SeverityLevel

barbeau reviewed Feb 4, 2021

View reviewed changes

Apply suggestions from code review

dae0143

Co-authored-by: Sean Barbeau <[email protected]>

lionel-nj added 3 commits February 4, 2021 13:53

revert changes to MissingTripEdgeNotice constructor

b795ac5

Merge branch 'errors-vs-warnings' of github.com:MobilityData/gtfs-val…

fae14b5

…idator into errors-vs-warnings

style: apply google format

4f8d846

lionel-nj requested a review from barbeau February 4, 2021 18:00

barbeau approved these changes Feb 4, 2021

View reviewed changes

barbeau merged commit 9a0ca92 into master Feb 4, 2021

barbeau deleted the errors-vs-warnings branch February 4, 2021 18:03

Conversation

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

Uh oh!

Uh oh!

lionel-nj commented Feb 2, 2021

Uh oh!

aababilov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lionel-nj commented Feb 3, 2021

Uh oh!

lionel-nj commented Feb 3, 2021

Uh oh!

aababilov commented Feb 4, 2021

Uh oh!

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

Uh oh!

Uh oh!

lionel-nj commented Feb 4, 2021

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

barbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lionel-nj commented Feb 2, 2021 •

edited

Loading

aababilov commented Feb 4, 2021 •

edited

Loading