Date: Parse literals, e.g., separators #692
Conversation
|
I looked at the diff and didn't find anything fishy. But I don't understand most of what's being changed there, so not sure how valuable that kind of review really is. |
|
Thanks for checking it out @jzaefferer, the goal is to fix #683 and the relevant change is this one https://github.com/globalizejs/globalize/pull/692/files#diff-4b6bbfed329023ed0ac60539e2ee7e4eR377 |
| @@ -122,10 +122,10 @@ QUnit.test( "should parse time presets", function( assert ) { | |||
| date.setSeconds( 7 ); | |||
| date = startOf( date, "second" ); | |||
| assertParseDate( assert, "5:35:07 PM", { time: "medium" }, date ); | |||
| assertParseDate( assert, "٥،٣٥،٠٧ م", { time: "medium" }, date, ar ); | |||
| assertParseDate( assert, "٥:٣٥:٠٧ م", { time: "medium" }, date, ar ); | |||
There was a problem hiding this comment.
Can you explain what this assertion is doing and what you changed here and why? That might help understand the bigger picture.
There was a problem hiding this comment.
In previous CLDR (when this test was created), ، was used as time separator. In recent CLDR, it was reverted to use : again. Although, the parser was buggy and didn't actually check for that. It was still passing because whatever character was working there. After the fix, this needed fix.
There was a problem hiding this comment.
Gotcha, thanks. Now this change makes a lot more sense.
|
Besides the "should parse invalid literal as null" test, there doesn't seem to be any new test to support "parse literals" here. Maybe there should be a new functional test for that? |
I've added an additional unit test a4fd84f. Now I ran out of ideas how to better test it. Thanks.
In the spirit of not repeating (unnecessary) tests (which I mostly learned from you 😝), I don't see how functional tests can help in this specific fix, but I'm open for ideas (in such case we can add it in a separate PR). |
|
Having one extra test seems fine. I don't understand what its doing though. |
Fixes #683