Repository navigation
Fix byUnicodePattern dropping an escaped quote inside a literal - #649
Conversation
dkhalanskyjb
left a comment
dkhalanskyjb
left a comment
There was a problem hiding this comment.
The fix looks correct, but the tests can be improved.
| assertEquals(time, format.parse(expected), pattern) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The test location isn't right. At the very least, it's not DateTimeFormatTest, as there are no dates involved. In my opinion, the best place to put this is UnicodeFormatTest. It's JVM-only, which reduces the coverage, but in exchange, we can write tests confirming that Java does exhibit the same behavior as our implementation.
|
Moved it to UnicodeFormatTest as a checkPattern case list, so each pattern is formatted and parsed by both this implementation and java.time and the strings compared. Removed from DateTimeFormatTest. jvmTest and apiCheck green on a fresh clone; the o''clock case fails on master without the fix. |
byUnicodePatterndrops the escaped quote inside a quoted literal. The Unicode pattern grammar spells an apostrophe inside a literal as'', and java.time formats'o''clock'aso'clock; here it formats asoclockand only parses that spelling. The parser walked the pattern by character, so on a'inside a literal it could not see whether the next character was another'.This walks the pattern by index and treats
''inside a literal as one apostrophe; a lone'still closes the literal. Adds a test over six pattern shapes.Found by an automated audit; the fix and the test were reviewed and verified by a person on a fresh clone with jvmTest, jsTest and wasmJsTest.