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

Normative: Allow calendar to determine choice of pattern #349

Merged
merged 1 commit into from Jan 9, 2020

Conversation

littledan
Copy link
Member

In CLDR, the set of patterns is based on the the calendar, not
simply the language-region-script. Previously, however, the set
was based purely on the base name. This patch switches to a two-level
lookup to permit implementations to have more accuracy.

Splitting off part of #227. Fixes part of #225.

Copy link
Contributor

@FrankYFTang FrankYFTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. This change is really needed as a bug fix and should merge ASAP.

@FrankYFTang
Copy link
Contributor

@anba @zbraniecki @caridy

@FrankYFTang FrankYFTang added bug c: datetime Component: dates, times, timezones Small Smaller change solvable in a Pull Request labels May 27, 2019
@littledan
Copy link
Member Author

I agree. We have also discussed it in TC39 and the ECMA-402 meeting in the past, so I don't think there is any formal need to request further consensus.

@FrankYFTang
Copy link
Contributor

what do we need to do to merge this PR? I would like to launch my implementation of #175 in v8 soon but I believe I need to adjust the code align with this PR first before launching that. This PR should be consider as a bug fix, right?
@sffc @leobalter

@FrankYFTang
Copy link
Contributor

hum... it looks like this PR is intertwine with #351
because once we select the pattern based on the calendar, patterns for calendar other than non-gregorian may have some fields we need to defined in #351 . So this PR cannot be merged without adding those field names (in the formatToParts) in these non-gregorian patterns.

@leobalter
Copy link
Member

I agree. We have also discussed it in TC39 and the ECMA-402 meeting in the past, so I don't think there is any formal need to request further consensus.

If possible can you help finding a reference to link from this PR? Just for clarity.

@leobalter
Copy link
Member

@FrankYFTang I believe it's fine to add this PR before #351, but I'm fine to wait if you prefer.

@littledan maybe you'd like to bring #351 to the TC39 meeting considering it complements unblocks this?

in the meantime, @FrankYFTang we need to get tests for #351 as well.

@FrankYFTang
Copy link
Contributor

The reason this PR is depending on #351 is because of the following.
Once we choose the pattern based on the calendar, then the pattern of non Gregorian calendar may contain relatedYear and yearName, therefore, the formatToParts will need to output these parts, but unless we also merge #351, we cannot output those parts. So this PR is blocked by #351 from my point of view.

@littledan
Copy link
Member Author

I think it's only intertwined in implementations that have a locale database that includes those patterns. IMO we can land this patch separately, and the spec will continue to imply that those patterns can't be used yet.

@FrankYFTang
Copy link
Contributor

I think it's only intertwined in implementations that have a locale database that includes those patterns. IMO we can land this patch separately, and the spec will continue to imply that those patterns can't be used yet.

agree. It should be a product launching dependency, not specification dependency. I think it is LGTM in this shape.

@leobalter leobalter added the has consensus Has consensus from TC39-TG2 label Sep 5, 2019
FrankYFTang added a commit to FrankYFTang/test262 that referenced this pull request Sep 27, 2019
Test DateTimeFormat change pattern based on calendar and output relatedYear and yearName
based on the assumption that "en-u-ca-chinese" will output 'relatedYear' and 'yearName'.
tc39/ecma402#349
tc39/ecma402#351
@rwaldron @leobalter @Ms2ger @littledan
backwardn pushed a commit to backwardn/v8 that referenced this pull request Oct 2, 2019
It is controlled by flag harmony_intl_other_calendars.
But this is also pretty intern-dependent with
harmony_intl_add_calendar_numbering_system and should be launched
all together to be meaningful.

tc39/ecma402#349
#349 Normative: Allow calendar to determine choice of pattern

tc39/ecma402#351
#351 Normative: Permit relatedYear and yearName in output



Bug: v8:9155
Change-Id: I67cd6bba6276bbb995186a9fe6202429d724ba61
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1588401
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63972}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 8, 2019
…nd permit relatedYear and yearName. r=jwalden

Implements the changes for these two spec PRs:
- tc39/ecma402#349
- tc39/ecma402#351

Restricted to Nightly because both PRs, despite having concensus, aren't yet merged into the spec.

Differential Revision: https://phabricator.services.mozilla.com/D41740

--HG--
extra : moz-landing-system : lando
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 8, 2019
…nd permit relatedYear and yearName. r=jwalden

Implements the changes for these two spec PRs:
- tc39/ecma402#349
- tc39/ecma402#351

Restricted to Nightly because both PRs, despite having concensus, aren't yet merged into the spec.

Differential Revision: https://phabricator.services.mozilla.com/D41740

UltraBlame original commit: abdfed2d3b35c6b7db7505748e426e48c771d553
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 8, 2019
…nd permit relatedYear and yearName. r=jwalden

Implements the changes for these two spec PRs:
- tc39/ecma402#349
- tc39/ecma402#351

Restricted to Nightly because both PRs, despite having concensus, aren't yet merged into the spec.

Differential Revision: https://phabricator.services.mozilla.com/D41740

UltraBlame original commit: abdfed2d3b35c6b7db7505748e426e48c771d553
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 8, 2019
…nd permit relatedYear and yearName. r=jwalden

Implements the changes for these two spec PRs:
- tc39/ecma402#349
- tc39/ecma402#351

Restricted to Nightly because both PRs, despite having concensus, aren't yet merged into the spec.

Differential Revision: https://phabricator.services.mozilla.com/D41740

UltraBlame original commit: abdfed2d3b35c6b7db7505748e426e48c771d553
leobalter pushed a commit to tc39/test262 that referenced this pull request Oct 8, 2019
Test DateTimeFormat change pattern based on calendar and output relatedYear and yearName
based on the assumption that "en-u-ca-chinese" will output 'relatedYear' and 'yearName'.
tc39/ecma402#349
tc39/ecma402#351
@rwaldron @leobalter @Ms2ger @littledan
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Oct 9, 2019
…nd permit relatedYear and yearName. r=jwalden

Implements the changes for these two spec PRs:
- tc39/ecma402#349
- tc39/ecma402#351

Restricted to Nightly because both PRs, despite having concensus, aren't yet merged into the spec.

Differential Revision: https://phabricator.services.mozilla.com/D41740
@leobalter
Copy link
Member

@littledan would you mind rebasing this one as well? I believe it's best we have your input here rather than I picking a preference without coordination.

@sffc
Copy link
Contributor

sffc commented Oct 25, 2019

@srl295 You commented in the September 26 meeting that it may be possible to write a test for this PR using the Hebrew calendar. Were you able to follow up with this?

@srl295
Copy link
Member

srl295 commented Nov 19, 2019

OK, i've just been around and around on this because of v8's "harmony_intl_other_calendars" flag. What is this flag and why is it off by default? @FrankYFTang

@srl295
Copy link
Member

srl295 commented Nov 19, 2019

@srl295 You commented in the September 26 meeting that it may be possible to write a test for this PR using the Hebrew calendar. Were you able to follow up with this?

So the original issue, now that I have found the right v8 flag, has a good test case. Examples shown below:

> new Date(0).toLocaleString("en-u-ca-chinese", {year: "numeric"})
'1969(ji-you)'
> new Date(0).toLocaleString("en-u-ca-gregory", {year: "numeric"})
'1969'
> new Date(0).toLocaleString("en-u-ca-japanese", {year: "numeric"})
'44 Shōwa'

The Chinese and Japanese calendars best match changes the pattern. You can use the ICU web demo to see this: (input pattern y, 3 different calendars).

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

@littledan
Copy link
Member Author

Now that this is rebased, and given the tests that @FrankYFTang wrote at tc39/test262#2379 , is this PR good to land?

@leobalter
Copy link
Member

@littledan please help figuring out the missing parts here? https://github.com/tc39/ecma402/wiki/Proposal-and-PR-Progress-Tracking#ecma-402-prs

Do we have a clear signal from SM?

I can merge to make it follow the same train as #351. Pinging @sffc for feedback

@sffc
Copy link
Contributor

sffc commented Nov 22, 2019

From last week ECMA-402:

SFC: Do we need a SpiderMonkey implementation for this one?

JSW: Off the top of my head I don't think we do (choose locales in this way), but I'm not sure.

@jswalden can you elaborate? Do we need a SM implementation for this PR? (Also #351)

@anba
Copy link
Contributor

anba commented Nov 28, 2019

#351 and #349 have been implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1573439, but are currently restricted to Firefox Nightly. We're currently in our soft freeze period, so we can't remove the Nightly-only restriction before next week. But when removed after this week, Firefox should be able to provide both changes for the Firefox 73 release.

@jswalden
Copy link
Collaborator

FWIW we removed the nightly-only restriction on #351 and #349 a couple days ago for SpiderMonkey.

@sffc
Copy link
Contributor

sffc commented Dec 12, 2019

FWIW we removed the nightly-only restriction on #351 and #349 a couple days ago for SpiderMonkey.

Great! Can you update the status wiki as appropriate?

@jswalden
Copy link
Collaborator

Can you update the status wiki as appropriate?

Done.

In CLDR, the set of patterns is based on the the calendar, not
simply the language-region-script. Previously, however, the set
was based purely on the base name. This patch switches to a two-level
lookup to permit implementations to have more accuracy.

Splitting off part of tc39#227. Fixes part of tc39#225.
@littledan
Copy link
Member Author

Rebased again; I believe this should be ready to land.

@leobalter leobalter merged commit 2bbcf5e into tc39:master Jan 9, 2020
ryanhaddad pushed a commit to WebKit/WebKit that referenced this pull request Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=209783

Reviewed by Keith Miller.

JSTests:

* stress/date-toLocaleString.js:
* stress/intl-datetimeformat.js:
Add tests and fix some existing ones.

* test262/config.yaml:
* test262/expectations.yaml:
Mark eight test cases passing...but skip half of them due to outdated CLDR data in macOS system ICU.

Source/JavaScriptCore:

This patch implements two intertwining normative changes to Intl.DateTimeFormat:
  - Calendar setting must be taken into account when choosing a date-time pattern
    tc39/ecma402#349
  - formatToParts must recognize relatedYear and yearName parts
    tc39/ecma402#349

* runtime/IntlDateTimeFormat.cpp:
(JSC::IntlDateTimeFormat::initializeDateTimeFormat):
(JSC::IntlDateTimeFormat::partTypeString):


Canonical link: https://commits.webkit.org/223435@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@260145 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: datetime Component: dates, times, timezones has consensus Has consensus from TC39-TG2 Small Smaller change solvable in a Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants