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: Add calendar and numberingSystem options #175

Open
wants to merge 2 commits into
base: master
from

Conversation

@littledan
Copy link
Member

commented Sep 13, 2017

This patch allows calendar and numberingSystem to be specified in
the options bag of the DateTimeFormat and NumberFormat constructors.
One use case for these options would be, when working with locales
which have two numbering systems and calendars in use--the UA default
may be the non-Western one, but in some contexts, it may be appropriate
to use the Western one. Currently, without the patch, it would be
necessary to parse the BCP 47 language tag in the application, but Intl
provides no library to do so. For example, this all occurs in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1370086

Related bug: #105 . This patch leaves out "collation" because of a lack
of clear use cases.

littledan added a commit to littledan/test262 that referenced this pull request Sep 13, 2017

intl tests for proposed additional options in options bag
This patch implements tests for the ECMA 402 PR at
tc39/ecma402#175

It is based on the test test/intl402/Collator/10.1.1_19_c.js
@littledan

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2017

@caridy

caridy approved these changes Sep 28, 2017

Copy link
Collaborator

left a comment

Seems to be very straight forward to add these new options.

@littledan

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2017

Any other opinions here? Can this be merged?

@littledan

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2017

In particular, do you think we should go to TC39 with this change, or is it minor enough to merge it without that presentation?

@caridy

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2017

I think we can do this without any presentation, but let's loop in few more folks to get their feedback:
@zbraniecki @jungshik

@zbraniecki

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2017

I agree with @caridy

@caridy

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2017

@littledan do you want to merge this?

1. If _calendar_ is not *undefined*, set _calendar_ to ? ToString(_calendar_).
1. Set _opt_.[[ca]] to _calendar_.
1. Let _numberingSystem_ be ? Get(_options_, *"numberingSystem"*).
1. If _numberingSystem_ is not *undefined*, set _numberingSystem_ to ? ToString(_numeringSystem_).

This comment has been minimized.

Copy link
@anba

anba Oct 24, 2017

Collaborator

Typo: numeringSystem (and in the line below and in the equivalent lines for Intl.NumberFormat.)

This comment has been minimized.

Copy link
@caridy

caridy Nov 16, 2017

Collaborator

@littledan can you update the PR to fix these typos?

This comment has been minimized.

Copy link
@littledan

littledan Jan 16, 2018

Author Member

Fixed the typo, apologies for the delay.

@caridy caridy added the s: discuss label Nov 16, 2017

@littledan littledan force-pushed the littledan:more-options branch from 01d962e to 8283f55 Jan 16, 2018

@littledan

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

This PR reached consensus at the November 2017 TC39 meeting, see notes.

@anba

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2018

Nit: Instead of

  1. Let calendar be ? Get(options, "calendar").
  2. If calendar is not undefined, set calendar to ? ToString(calendar).
  3. Set opt.[[ca]] to calendar.

The GetOption operation should be used for consistency with access to other options.

  1. Let calendar be ? GetOption(options, "calendar", "string", undefined, undefined).
  2. Set opt.[[ca]] to calendar.

(And we should change Get(options, "timeZone") in InitializeDateTimeFormat to use GetOption, too.)

But probably more important than how to access and convert the new options: Currently all options-bag options seem to be validated in some way and invalid option values are rejected with an error, whereas invalid options from Unicode extensions are simply ignored (e.g. Intl.Collator("de", {caseFirst: "invalid"}) throws a RangeError, whereas Intl.Collator("de-u-kf-invalid") is silently accepted). This change introduces the first options-bag options which don't get any validation at all, which seems a bit unfortunate. And furthermore we should also formally define the allowed values for the "numberingSystem" and "calendar" options (BCP 47 vs. UTS 35 names).

For example let's consider the "calendar" option:
https://tc39.github.io/ecma402/#sec-properties-of-intl-datetimeformat-instances describes [[Calendar]] as:

[[Calendar]] is a String value with the "type" given in Unicode Technical Standard 35 for the calendar used for formatting.

But which "type" is meant here? The value of the "name" attribute of the <type> element for BCP 47 (https://unicode.org/repos/cldr/tags/latest/common/bcp47/calendar.xml) or the value of "type" attribute of the <calendar> element (https://unicode.org/repos/cldr/tags/latest/common/main/root.xml)? And is it then Intl.DateTimeFormat("de", {calendar: "gregory"}) or Intl.DateTimeFormat("de", {calendar: "gregorian"}) or maybe even both? Also what about canonicalizing the input, for example if the input is "GrEgOrIaN", does it get accepted and if it is accepted, does it get canonicalized to "gregorian"? And if we only accept known calendars per UTS 35, we probably also need to explicitly disallow the name "generic", don't we?

For example Intl.DateTimeFormat("de-u-ca-gregory").resolvedOptions().calendar prints "gregory" for me with V8 and SpiderMonkey, but "gregorian" with JSC (on Ubuntu16.04, IIRC JSC uses the system ICU which is ICU55 for Ubuntu16.04; V8 and SpiderMonkey both use ICU60). Right now I'd say only JSC is correct here and "gregorian" is the expected output. But WDYT?

@srl295

srl295 approved these changes Jan 19, 2018

Copy link
Member

left a comment

sounds good to me

@littledan

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

Unfortunately I don't think ICU has an API for validating these sorts of things. I was thinking of the validation logic a bit like, things which were based on an open-ended name would be checked through resolvedOptions.

Do you think it will be important for users to get a RangeError here? Does anyone remember the original intention with the current design for error checking? cc @NorbertLindenberg

We reviewed this patch at the ECMA 402 meeting. At that time, I'd only skimmed @anba's comment and didn't realize the validation issue. I guess this patch isn't ready to merge until we make a decision on that question.

@zbraniecki

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2018

As discussed on the ECMA402 call - we should only validate the options to be well-formed, just like we do for extension key values.

@littledan littledan force-pushed the littledan:more-options branch from 8283f55 to 98bcc48 Feb 27, 2018

littledan added a commit to littledan/ecma402 that referenced this pull request Feb 27, 2018

Normative: Add calendar and numberingSystem options
This patch allows calendar and numberingSystem to be specified in
the options bag of the DateTimeFormat and NumberFormat constructors.
One use case for these options would be, when working with locales
which have two numbering systems and calendars in use--the UA default
may be the non-Western one, but in some contexts, it may be appropriate
to use the Western one. Currently, without the patch, it would be
necessary to parse the BCP 47 language tag in the application, but Intl
provides no library to do so. For example, this all occurs in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1370086

This patch validates the calendar and numbering system by comparing
them to the grammar allowed for Unicode extension tags, per the resolution
documented at tc39#175 (comment)

Related bug: tc39#105 . This patch leaves out "collation" because of a lack
of clear use cases.
@littledan

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2018

OK, the newly uploaded patch is based on validating the grammar, but not contents, of the calendar and numberFormat options. It does not address @anba's other canonicalization question, however. How does this seem?

@littledan littledan force-pushed the littledan:more-options branch from 98bcc48 to 3925958 Feb 27, 2018

littledan added a commit to littledan/ecma402 that referenced this pull request Feb 27, 2018

Normative: Add calendar and numberingSystem options
This patch allows calendar and numberingSystem to be specified in
the options bag of the DateTimeFormat and NumberFormat constructors.
One use case for these options would be, when working with locales
which have two numbering systems and calendars in use--the UA default
may be the non-Western one, but in some contexts, it may be appropriate
to use the Western one. Currently, without the patch, it would be
necessary to parse the BCP 47 language tag in the application, but Intl
provides no library to do so. For example, this all occurs in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1370086

This patch validates the calendar and numbering system by comparing
them to the grammar allowed for Unicode extension tags, per the resolution
documented at tc39#175 (comment)

Related bug: tc39#105 . This patch leaves out "collation" because of a lack
of clear use cases.
@caridy

This comment has been minimized.

Copy link
Collaborator

commented Feb 27, 2018

hmm, at first glance, "match type" feels weird to me considering that you will have to open the link, and try to get the glimpse of what type is. This seems to be simple enough that maybe we can specify it here.

@sffc sffc added the small label Apr 19, 2019

@zbraniecki

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

SpiderMonkey does not have a bug filled yet, and would be a great first-bug if anyone is interested in a small task to help us push this spec forward. I'm happy to mentor.

@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

<emu-alg>
1. Let _requestedLocales_ be ? CanonicalizeLocaleList(_locales_).
1. Let _options_ be ? ToDateTimeOptions(_options_, `"any"`, `"date"`).
1. Let _opt_ be a new Record.
1. Let _matcher_ be ? GetOption(_options_, `"localeMatcher"`, `"string"`, &laquo; `"lookup"`, `"best fit"` &raquo;, `"best fit"`).
1. Set _opt_.[[localeMatcher]] to _matcher_.
1. Let _calendar_ be ? GetOption(_options_, `"calendar"`, `"string"`, *undefined*, *undefined*).
1. If _calendar_ does not match `type`, throw a *RangeError* exception.

This comment has been minimized.

Copy link
@FrankYFTang

FrankYFTang Apr 19, 2019

Collaborator

"does not match type" is not implementable. I propose we follow https://tc39.github.io/proposal-intl-locale/#sec-Intl.Locale and change it to

        1. If _calendar_ is not *undefined*, then
          1. If _calendar_ does not match the `(3*8alphanum) *("-" (3*8alphanum))` sequence, throw a *RangeError* exception.
1. If _calendar_ does not match `type`, throw a *RangeError* exception.
1. Set _opt_.[[ca]] to _calendar_.
1. Let _numberingSystem_ be ? GetOption(_options_, `"numberingSystem"`, `"string"`, *undefined*, *undefined*).
1. If _numberingSystem_ does not match `type`, throw a *RangeError* exception.

This comment has been minimized.

Copy link
@FrankYFTang

FrankYFTang Apr 19, 2019

Collaborator

Again, "does not match type" is not implementable. I propose we follow https://tc39.github.io/proposal-intl-locale/#sec-Intl.Locale and change it to

        1. If _numberingSystem_ is not *undefined*, then
          1. If _numberingSystem_ does not match the `(3*8alphanum) *("-" (3*8alphanum))` sequence, throw a *RangeError* exception.
@@ -49,6 +53,9 @@ <h1>InitializeNumberFormat ( _numberFormat_, _locales_, _options_ )</h1>
1. Let _opt_ be a new Record.
1. Let _matcher_ be ? GetOption(_options_, `"localeMatcher"`, `"string"`, &laquo; `"lookup"`, `"best fit"` &raquo;, `"best fit"`).
1. Set _opt_.[[localeMatcher]] to _matcher_.
1. Let _numberingSystem_ be ? GetOption(_options_, `"numberingSystem"`, `"string"`, *undefined*, *undefined*).
1. If _numberingSystem_ does not match `type`, throw a *RangeError* exception.

This comment has been minimized.

Copy link
@FrankYFTang

FrankYFTang Apr 19, 2019

Collaborator

Again, "does not match type" is not implementable. I propose we follow https://tc39.github.io/proposal-intl-locale/#sec-Intl.Locale and change it to

        1. If _numberingSystem_ is not *undefined*, then
          1. If _numberingSystem_ does not match the `(3*8alphanum) *("-" (3*8alphanum))` sequence, throw a *RangeError* exception.

@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

Should this also apply to https://github.com/tc39/proposal-intl-relative-time ?

@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

How about "collation" for Intl.Collator ?

@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented Apr 19, 2019

v8 implementation in https://chromium-review.googlesource.com/c/v8/v8/+/1575919
Have some issue of the condition of throwing RangeError in the spec.

@littledan

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

I agree with all of your comments. We should make these changes in this PR. Cc @Ms2ger.

littledan added some commits Sep 13, 2017

Normative: Add calendar and numberingSystem options
This patch allows calendar and numberingSystem to be specified in
the options bag of the DateTimeFormat and NumberFormat constructors.
One use case for these options would be, when working with locales
which have two numbering systems and calendars in use--the UA default
may be the non-Western one, but in some contexts, it may be appropriate
to use the Western one. Currently, without the patch, it would be
necessary to parse the BCP 47 language tag in the application, but Intl
provides no library to do so. For example, this all occurs in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1370086

This patch validates the calendar and numbering system by comparing
them to the grammar allowed for Unicode extension tags, per the resolution
documented at #175 (comment)

Related bug: #105 . This patch leaves out "collation" because of a lack
of clear use cases.

@littledan littledan force-pushed the littledan:more-options branch from 3925958 to 1bda139 Apr 23, 2019

@littledan

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Collation is omitted with a lack of clear use cases, following discussion in #105 (comment) . I agree we should apply this to Intl.RelativeTimeFormat to take a numberingSystem option and plan to follow up with that. Otherwise, I've updated this PR based on the comments.

littledan added a commit to tc39/proposal-intl-relative-time that referenced this pull request Apr 23, 2019

@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

v8 implementation is checked in under flag --harmony-intl-add-calendar-numbering-system It will be nice if someone can contribute tests to test262.

joyeecheung pushed a commit to joyeecheung/v8 that referenced this pull request May 9, 2019

[Intl] Add numberingSystem/calendar
Implement ECMA402 PR tc39/ecma402#175
Add numberingSystem option to NumberFormat
And numberingSystem and calendar option to DateTimeFormat


Bug: v8:9154
Change-Id: Ic4e85a232a9ad26c17ee20385f839b0e09a56c77
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1575919
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61061}

leobalter added a commit to tc39/test262 that referenced this pull request May 16, 2019

intl tests for proposed additional options in options bag (#1225)
This patch implements tests for the ECMA 402 PR at
tc39/ecma402#175

It is based on the test test/intl402/Collator/10.1.1_19_c.js
@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

Could we now consider this PR reach "stage 3" status?

@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

If we all agree this PR reach stage 3 status, I will ask to ship it in v8 w/ m77. The two tests in test262 verify it are in
test262/intl402/NumberFormat/numbering-system-options
test262/intl402/DateTimeFormat/numbering-system-calendar-options

V8 is now implemented behind the flag --harmony-intl-add-calendar-numbering-system

uuhan pushed a commit to uuhan/v8 that referenced this pull request May 27, 2019

Stage calendar/numberingSystem options.
Adds
"calendar" and "numberingSystem" options for Intl.DateTimeForamt and
"numberingSystem" for Intl.NumberFormat.

Plan to flip to ship in early June (after m76 branch) for chrome m77.
Spec: tc39/ecma402#175
I2I: https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!searchin/v8-dev/ftang%7Csort:date/v8-dev/7sk-rEHuCY4/n7kH0WzyAwAJ

Tests:
test262/intl402/NumberFormat/numbering-system-options
test262/intl402/DateTimeFormat/numbering-system-calendar-options
intl/number-format/check-numbering-system
intl/date-format/check-numbering-system
intl/date-format/check-calendar

Bug: v8:9154
Change-Id: I80020b9af6bf9c87f5a1efc7aac3080e723eea34
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1622728
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61759}
@FrankYFTang

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

I will ask to ship it in v8 w/ m77.

sorry. I missed the deadline for m77. I can only try it again for m78.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.