feat: Treat NonAsciiOrNonPrintableCharNotice as a warning instead of error#728
Conversation
|
Friendly ping here! That's a blocker because right now our upstream validator rejects valid feeds that have non-ASCII characters in IDs. |
barbeau
left a comment
barbeau
left a comment
There was a problem hiding this comment.
Thanks @aababilov! Converting this to a warning LGTM, as the spec says it's recommended (but not required) to use printable ASCII:
Using only printable ASCII characters is recommended.
A few comments in-line
| @@ -0,0 +1,58 @@ | |||
| /* | |||
There was a problem hiding this comment.
Note that this has a dependency on #729 as both commits and included here (although I believe the new test for NonAsciiOrNonPrintableCharNotice is missing). Can that dependency be removed so we can merge this separately?
There was a problem hiding this comment.
Done. My bad, GitHub is a bit challenging when you have dependent pull requests :)
| "csvRowNumber", csvRowNumber, | ||
| "columnName", columnName)); | ||
| "columnName", columnName), | ||
| SeverityLevel.INFO); |
There was a problem hiding this comment.
Shouldn't this be SeverityLevel.WARNING?
| row.getFileName(), row.getRowNumber(), row.getColumnName(columnIndex))); | ||
| } | ||
| if (s != null) { | ||
| final String trimmed = s.strip(); |
There was a problem hiding this comment.
Nit - given that there is also a Java method trim(), this variable is probably better named stripped.
There was a problem hiding this comment.
Done.
By the way, can you tell the difference between "strip" and "trim" as a native English speaker? To me, these words are interchangeable and one must check the documentation to understand the difference :)
There was a problem hiding this comment.
Actually, that change belongs to another pull request. Fixed.
There was a problem hiding this comment.
By the way, can you tell the difference between "strip" and "trim" as a native English speaker?
Not in the context of Java :). In general use "strip" has somewhat of a more aggressive and coarse connotation than "trim". "trim" implies a more precise and neat action.
barbeau
changed the title
Issue MobilityData#549 An error in RowParser stops validation for the whole feed. However, if ID has non-printable or non-ASCII characters, the feed may be still successfully parsed and served to the end user. We are lowering priority of NonAsciiOrNonPrintableCharNotice to WARNING and modify RowParser so that it checks severity level of its notices. We also simplify the code of asId function and reduce amount of calls to row.asString from 3 to 1.
aababilov
force-pushed
the
errors-in-row-parser
branch
from
163fe26 to
e9a5462
Compare
|
Thank you for review! |
2 participants
Issue #549
An error in RowParser stops validation for the whole feed. However, if
ID has non-printable or non-ASCII characters, the feed may be still
successfully parsed and served to the end user.
We are lowering priority of NonAsciiOrNonPrintableCharNotice to WARNING
and modify RowParser so that it checks severity level of its notices.
We also simplify the code of asId function and reduce amount of calls to
row.asString from 3 to 1.