search

globalizejs / globalize

A JavaScript library for internationalization and localization that leverages the official Unicode CLDR JSON data
https://globalizejs.com
MIT License
4.8k stars 603 forks source link

Date: Parse literals, e.g., separators #692

Closed rxaviers closed 7 years ago

rxaviers commented 7 years ago

Fixes #683

jzaefferer commented 7 years ago

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.

rxaviers commented 7 years ago

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

jzaefferer commented 7 years ago

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?

rxaviers commented 7 years ago

Besides the "should parse invalid literal as null" test, there doesn't seem to be any new test to support "parse literals" here.

I've added an additional unit test a4fd84f. Now I ran out of ideas how to better test it. Thanks.

Maybe there should be a new functional test for that?

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

jzaefferer commented 7 years ago

Having one extra test seems fine. I don't understand what its doing though.