Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix zh-Hant - PM "afternoon" handled correctly #508

Closed

Conversation

jspeaker
Copy link

Fixed zh-Hant PM recognition to include 'afternoon' as it alphabetically comes before 'am' - fixes issue where AM and PM were both parsing as the AM datetime because "afternoon" was not being recognized as PM and therefore not adding 12 hours.

@rxaviers
Copy link
Member

Thank you for your contribution. Please, could you include a test case that passes after the fix, but fails without it?

Thanks

@jspeaker
Copy link
Author

It appears that you use QUnit on this project, and my familiarity is mostly in the Jasmine world… it would take me a bit to get set up to run said tests, I’m supposing, and I’ve got my hands full with the work I’m actively doing (that required this fix).

I’ll flag this email and if I get “free time,” then certainly.

Also, it appeared additionally that no tests have been created in the project’s date tests that specifically switch locales. It seems that the best bet would be to refactor the date tests to run the full set of assertions based on every supported locale rather than just the currently hardcoded “en”.

Thoughts?

Cheers

Jim Speaker

From: Rafael Xavier de Souza [mailto:[email protected]]
Sent: Saturday, September 12, 2015 11:29 AM
To: jquery/globalize
Cc: Jim Speaker
Subject: Re: [globalize] Fix zh-Hant - PM "afternoon" handled correctly (#508)

Thank you for your contribution. Please, could you include a test case that passes after the fix, but fails without it?

Thanks


Reply to this email directly or view it on GitHub #508 (comment) . https://github.com/notifications/beacon/AAyc15TuX3U8IClFvsBnS3mZECERHSGXks5oxGZlgaJpZM4F8TNc.gif

@jspeaker jspeaker force-pushed the fix-zh-Hant-parseDate-PM branch 2 times, most recently from ffd4453 to 8bbb8f4 Compare September 12, 2015 18:59
Fixes globalizejs#509
Fixed zh-Hant PM recognition to include 'afternoon' as it alphabetically comes before 'am' - fixes issue where AM and PM were both parsing as the AM datetime because "afternoon" was not being recognized as PM and therefore not adding 12 hours.
omes before 'am' - fixes issue where AM and PM were both parsing as the AM date
@jspeaker jspeaker force-pushed the fix-zh-Hant-parseDate-PM branch 4 times, most recently from bd02095 to ee61d8b Compare September 12, 2015 19:27
@jspeaker jspeaker force-pushed the fix-zh-Hant-parseDate-PM branch from ee61d8b to 519992a Compare September 12, 2015 19:28
@rxaviers
Copy link
Member

No problem. So, please could you provide me a couple of Dates where you see this problem happening for the zh-Hant locale?

@jspeaker
Copy link
Author

FYI, as you can see, I had trouble with the check-in comment format on my pull request, tried many times, and finally threw up my hands.

Any time with the PM characters (下午), which are the same as “afternoon,” will cause this failure, as it will be evaluated as AM.

Globalize.parseDate("下午1:00", { time: "short" });

The following test fails with the original code, and passes with the updated code.

The failure is as follows:

Expected Date(Tue Sep 15 2015 01:00:00 GMT-0700 (Pacific Daylight Time)) to equal Date(Tue Sep 15 2015 13:00:00 GMT-0700 (Pacific Daylight Time)).

Original code:

  if ( hour12 && amPm === "pm" ) {

      date.setHours( date.getHours() + 12 );

  }

Updated code:

 if ( hour12 && ( amPm === "pm" || amPm === "afternoon" ) ) {

     date.setHours( date.getHours() + 12 );

 }

And, here’s the jasmine test for this zh-Hant time problem.

describe("Given globalization date module as zh-Hant", function () {

var language = "zh-Hant", actual, expected = new Date(2015, 8, 15, 13, 0, 0, 0);

beforeEach(function (done) {

$.when(

    $.getJSON("/úæʒ_Scripts/globalization/cldr/main/zh-Hant/numbers.json"),

    $.getJSON("/úæʒ_Scripts/globalization/cldr/supplemental/numberingSystems.json"),

    $.getJSON("/úæʒ_Scripts/globalization/cldr/main/zh-Hant/ca-gregorian.json"),

    $.getJSON("/úæʒ_Scripts/globalization/cldr/supplemental/timeData.json"),

    $.getJSON("/úæʒ_Scripts/globalization/cldr/supplemental/likelySubtags.json")

  )

  .then(function () {

    return [].slice.apply(arguments, [0]).map(function (result) {

      return result[0];

    });

  })

  .then(Globalize.load)

  .then(

    function () {

      Globalize(language);

      Globalize.locale(language);

      done();

    }

  );

});

describe("When parsing time", function() {

beforeEach(function () {

  actual = Globalize.parseDate("下午1:00", { time: "short" });

});



it("Then it should equal todays date with the correct PM time", function () {

  expect(actual).toEqual(expected);

});

});

});

Sorry I don’t have the time to dig into QUnit. I’m sure it’s no big deal, but I need to get back to work now.

Oh, and by the way, I absolutely love the Globalize library. I’m using globalize.validate with jquery.validate, and incorporated globalize into some of my custom validators – it’s a God send for the multi-lingual web application I’m working on. The timing of the 1.0 release was right on the money with my need for it.

Thank you very much!

Jim Speaker

From: Rafael Xavier de Souza [mailto:[email protected]]
Sent: Tuesday, September 15, 2015 3:24 AM
To: jquery/globalize
Cc: Jim Speaker
Subject: Re: [globalize] Fix zh-Hant - PM "afternoon" handled correctly (#508)

No problem. So, please could you provide me a couple of Dates where you see this problem happening for the zh-Hant locale?


Reply to this email directly or view it on GitHub #508 (comment) . https://github.com/notifications/beacon/AAyc1_igKf4yGZrjKkTam9JkBd1MG6Voks5ox-k6gaJpZM4F8TNc.gif

@jspeaker
Copy link
Author

Oh, one more thing, this will also fix the same problem with zh (aka zh-Hans). The "afternoon" characters exist in both, though they are ofc different characters in zh-Hans.

@rxaviers
Copy link
Member

Any time with the PM characters (下午), which are the same as “afternoon,” will cause this failure, as it will be evaluated as AM.

Excellent, that's what I needed. Thanks. Now I need to find time to add the unit test, so we merge this. Alternatively, if you want to give it a try, the unit tests should go here and we'll need to load data for the zh-Hant data analogous to this code and create an zhHant instance analogous to this code.

@rxaviers
Copy link
Member

@jspeaker the implementation unfortunately isn't correct and needs considerable additions to behave correctly. Reviewing this, I found out that:

  • afternoon is locale-dependent, i.e., the definition changes from locale to locale;
  • afternoon is not the only keyword, there are also evening, midnight, night, and noon. Also they are numbered and occasionally there are two variants like afternoon1 and afternoon2.
  • Not all afternoonss means it's +12. For example, for uz afternoon means it's from 11am and on (link). Therefore, assuming we can always +12 when we find afternoon is wrong.

Ref: http://www.unicode.org/reports/tr35/tr35-dates.html#Day_Period_Rule_Sets

rxaviers added a commit to rxaviers/globalize that referenced this pull request Mar 2, 2017
CLDR introduced additionsl dayPeriods (e.g., morning1, afternoon1,
evening1, etc) along with am and pm. The new dayPeriods should be handle
by "b" and "B" date patterns, while the "a" pattern should still handle
am/pm.

For Chinese, the corresponding value for either pm or afternoon1 is
exactly the same, therefore it causes problem on the reverse lookup when
parsing.

This change fixes the above parsing issue by filtering am/pm only in the
parser properties. It also filters it out on formatting as a simple
optimization (to avoid unnecessary properties).

Fixes globalizejs#509
Closes globalizejs#508
rxaviers added a commit to rxaviers/globalize that referenced this pull request Mar 2, 2017
CLDR introduced additional dayPeriods (e.g., morning1, afternoon1,
evening1, etc) along with am and pm. The new dayPeriods should be handle
by "b" and "B" date patterns, while the "a" pattern should still handle
am/pm.

For Chinese, the corresponding value for either pm or afternoon1 is
exactly the same, therefore it causes problem on the reverse lookup when
parsing.

This change fixes the above parsing issue by filtering am/pm only in the
parser properties. It also filters it out on formatting as a simple
optimization (to avoid unnecessary properties).

Fixes globalizejs#509
Closes globalizejs#508
rxaviers added a commit that referenced this pull request Mar 10, 2017
CLDR introduced additional dayPeriods (e.g., morning1, afternoon1,
evening1, etc) along with am and pm. The new dayPeriods should be handle
by "b" and "B" date patterns, while the "a" pattern should still handle
am/pm.

For Chinese, the corresponding value for either pm or afternoon1 is
exactly the same, therefore it causes problem on the reverse lookup when
parsing.

This change fixes the above parsing issue by filtering am/pm only in the
parser properties. It also filters it out on formatting as a simple
optimization (to avoid unnecessary properties).

Fixes #509
Closes #508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants