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: Canonicalize value of extension in ApplyUnicodeExtensionToTag #846

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

FrankYFTang
Copy link
Contributor

Close #707

@FrankYFTang FrankYFTang changed the title Canonicalize value of extension in ApplyUnicodeExtensionToTag Normative: Canonicalize value of extension in ApplyUnicodeExtensionToTag Dec 14, 2023
@FrankYFTang
Copy link
Contributor Author

@trflynn89

@trflynn89
Copy link
Contributor

Thanks! I tested this change over in Serenity's LibJS and it looks good to me:

> const loc = new Intl.Locale('en', {calendar: 'islamicc'});

> loc.toString()
"en-u-ca-islamic-civil"

> loc.calendar
"islamic-civil"

We also now pass test/intl402/Locale/constructor-options-canonicalized.js with this change, which was previously failing on this islamicc case.

Copy link
Contributor

@anba anba left a comment

Choose a reason for hiding this comment

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

👍 LGTM.

@@ -136,7 +136,8 @@ <h1>
1. Let _optionsValue_ be _options_.[[&lt;_key_&gt;]].
1. If _optionsValue_ is not *undefined*, then
1. Assert: Type(_optionsValue_) is String.
1. Set _value_ to _optionsValue_.
1. Let _optionsUValue_ be the ASCII-lowercase of _optionsValue_.
1. Set _value_ to the String value resulting from canonicalizing _optionsUValue_ as a value of key _key_ per <a href="https://unicode.org/reports/tr35/#processing-localeids">Unicode Technical Standard #35 Part 1 Core, Annex C LocaleId Canonicalization Section 5 Canonicalizing Syntax, Processing LocaleIds</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

ResolveLocale also explicitly converts key to ASCII lower-case before this step. Should we do the same here? (key is already in ASCII lower-case, so it's not strictly required to perform any case conversions, neither here nor in ResolveLocale.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In here, Inside 14.1.3 ApplyUnicodeExtensionToTag ( tag, options, relevantExtensionKeys )
https://tc39.es/ecma402/#sec-apply-unicode-extension-to-tag
5. For each element key of relevantExtensionKeys, do

and only place call it is in
14.1.1 Intl.Locale ( tag [ , options ] )
https://tc39.es/ecma402/#sec-Intl.Locale
29. Let r be ApplyUnicodeExtensionToTag(tag, opt, relevantExtensionKeys).

where relevantExtensionKeys is from
2. Let relevantExtensionKeys be %Locale%.[[RelevantExtensionKeys]].

and %Locale%.[[RelevantExtensionKeys]] is defined in
https://tc39.es/ecma402/#sec-intl.locale-internal-slots
"The value of the [[RelevantExtensionKeys]] internal slot is « "ca", "co", "hc", "kf", "kn", "nu" ». If %Collator%.[[RelevantExtensionKeys]] does not contain "kf", then remove "kf" from %Locale%.[[RelevantExtensionKeys]]. If %Collator%.[[RelevantExtensionKeys]] does not contain "kn", then remove "kn" from %Locale%.[[RelevantExtensionKeys]]."

I agree with you that the "converts key to ASCII lower-case" in ResolveLocale is also unnecessary. But that is another editoral issue we can change separately

@littledan
Copy link
Member

So the rule here is, "canonicalize if we recognize something, but if there's an unrecognized value, let it continue as uninterpreted text" right? And because of this latter part, we can't convert things like first day of the week into a number in this place, right? I'm OK with this PR, but if you could summarize how these things fit together, that'd help my understanding.

@ben-allen ben-allen merged commit f7983ff into tc39:master Feb 5, 2024
2 checks passed
@FrankYFTang
Copy link
Contributor Author

Ben- why do you merge this PR now. This PR is on the agenda for TC39 discussion tomorrow https://github.com/tc39/agendas/blob/main/2024/02.md we should not merge it until TC39 agree with it first.

@FrankYFTang
Copy link
Contributor Author

This is on the agenda for discussion on 2024-02-06 TC39 meeting https://github.com/tc39/agendas/blob/main/2024/02.md

ben-allen added a commit that referenced this pull request Feb 5, 2024
@ben-allen
Copy link
Contributor

oh my goodness, sorry about that! I've pushed a revert commit for it.

@FrankYFTang
Copy link
Contributor Author

Has concensus from TG1 this morning around 11:25am PST (2024-02-06)

Please reland this. @ben-allen

@FrankYFTang FrankYFTang deleted the canonExt branch February 6, 2024 19:32
@trflynn89
Copy link
Contributor

Hi @ben-allen - friendly ping, can the commit be restored? Thanks!

FrankYFTang added a commit to FrankYFTang/test262 that referenced this pull request Feb 23, 2024
the calenar name should be canonicalized
Spec change tc39/ecma402#846
ptomato pushed a commit to tc39/test262 that referenced this pull request Feb 23, 2024
the calenar name should be canonicalized
Spec change tc39/ecma402#846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Previously Discussed
Development

Successfully merging this pull request may close these issues.

ApplyUnicodeExtensionToTag and ResolveLocale set the result record's internal slots to non-canonical values
8 participants