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 intl-displaynames-v2 #622

Merged
merged 11 commits into from
Jan 12, 2022

Conversation

FrankYFTang
Copy link
Contributor

Based on https://tc39.es/intl-displaynames-v2/

pending on TC39 2021-Dec decision

Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM overall, please check out my feedback inline.

spec/displaynames.html Outdated Show resolved Hide resolved
spec/displaynames.html Show resolved Hide resolved
spec/displaynames.html Show resolved Hide resolved
<li>The display name style fields under display name type *"language"* should contain Records with keys corresponding to language codes matching the `unicode_language_id` production. The value of these fields must be string values.</li>
<li>The display name style fields under display name type *"region"* should contain Records with keys corresponding to region codes. The value of these fields must be string values.</li>
<li>The display name style fields under display name type *"script"* should contain Records with keys corresponding to script codes. The value of these fields must be string values.</li>
<li>The display name style fields under display name type *"currency"* should contain Records with keys corresponding to currency codes. The value of these fields must be string values.</li>
<li>The display name styles fields under display name type `"calendar"` should contain Records, with keys corresponding to a String value with the `type` given in Unicode Technical Standard 35 for the calendar used for formatting. The value of these fields must be string values.</li>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li>The display name styles fields under display name type `"calendar"` should contain Records, with keys corresponding to a String value with the `type` given in Unicode Technical Standard 35 for the calendar used for formatting. The value of these fields must be string values.</li>
<li>The display name style fields under display name type `"calendar"` should contain Records with keys corresponding to a String value with the `type` given in Unicode Technical Standard 35 for the calendar used for formatting. The value of these fields must be string values.</li>

Copy link
Member

Choose a reason for hiding this comment

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

It seems like you say "string" twice here, so maybe that could be fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "styles fields" were referring to those fields inside typeFields "26. Let styleFields be typeFields.[[<style>]].". If you look at the spec text before, I wrote the same way. If we want to change it, I think we should make an editorial changes on ECMA402 on the preexisting one separated from here.

<li>The display name style fields under display name type *"language"* should contain Records with keys corresponding to language codes matching the `unicode_language_id` production. The value of these fields must be string values.</li>
<li>The display name style fields under display name type *"region"* should contain Records with keys corresponding to region codes. The value of these fields must be string values.</li>
<li>The display name style fields under display name type *"script"* should contain Records with keys corresponding to script codes. The value of these fields must be string values.</li>
<li>The display name style fields under display name type *"currency"* should contain Records with keys corresponding to currency codes. The value of these fields must be string values.</li>
<li>The display name styles fields under display name type `"calendar"` should contain Records, with keys corresponding to a String value with the `type` given in Unicode Technical Standard 35 for the calendar used for formatting. The value of these fields must be string values.</li>
<li>The display name styles fields under display name type `"dateTimeField"` should contain Records, with keys corresponding to codes listed in <emu-xref href="#table-validcodefordatetimefield"></emu-xref>. The value of these fields must be string values.</li>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li>The display name styles fields under display name type `"dateTimeField"` should contain Records, with keys corresponding to codes listed in <emu-xref href="#table-validcodefordatetimefield"></emu-xref>. The value of these fields must be string values.</li>
<li>The display name style fields under display name type `"dateTimeField"` should contain Records with keys corresponding to codes listed in <emu-xref href="#table-validcodefordatetimefield"></emu-xref>. The value of these fields must be string values.</li>

@ryzokuken
Copy link
Member

One more thing: since this is a new addition, should this use the new structured argument syntax for new AOs? Otherwise it would increase the amount of editorial work.

spec/displaynames.html Outdated Show resolved Hide resolved
spec/displaynames.html Outdated Show resolved Hide resolved
Correct incorrect insertion point of proposed changes
@FrankYFTang
Copy link
Contributor Author

One more thing: since this is a new addition, should this use the new structured argument syntax for new AOs? Otherwise it would increase the amount of editorial work.

you are talking about IsValidDateTimeFieldCode right? I will change it accordingly later today.

@FrankYFTang
Copy link
Contributor Author

One more thing: since this is a new addition, should this use the new structured argument syntax for new AOs? Otherwise it would increase the amount of editorial work.

DONE

@FrankYFTang
Copy link
Contributor Author

PTAL

spec/displaynames.html Outdated Show resolved Hide resolved
spec/displaynames.html Outdated Show resolved Hide resolved
spec/displaynames.html Outdated Show resolved Hide resolved
spec/displaynames.html Outdated Show resolved Hide resolved
FrankYFTang and others added 4 commits December 14, 2021 16:14
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
@FrankYFTang
Copy link
Contributor Author

@gibson042 @ryzokuken Is there any action I need to take for this PR?

Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM overall, just that old comment about rewording one sentence, would be nice if you addressed it.

spec/displaynames.html Show resolved Hide resolved
@ryzokuken ryzokuken changed the title Stage4 PR for intl-displaynames-v2 Normative: add intl-displaynames-v2 Jan 6, 2022
@FrankYFTang
Copy link
Contributor Author

PTAL

@ryzokuken
Copy link
Member

Looks great, @gibson042 would you like to review this before we hit merge?

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

No firm objection to merging as-is, but I do have a question about what seems like a mistake.

@@ -68,8 +151,13 @@ <h1>Intl.DisplayNames ( _locales_, _options_ )</h1>
1. Let _dataLocaleData_ be _localeData_.[[&lt;_dataLocale_&gt;]].
1. Let _types_ be _dataLocaleData_.[[types]].
1. Assert: _types_ is a Record (see <emu-xref href="#sec-Intl.DisplayNames-internal-slots"></emu-xref>).
1. Let _languageDisplay_ be ? GetOption(_options_, *"languageDisplay"*, *"string"*, &laquo; *"dialect"*, *"standard"* &raquo;, *"dialect"*).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to read languageDisplay even if type is not "language"? The consequences are observable to ECMAScript code, e.g.

new Intl.DisplayNames(undefined, {
  type: "currency",
  get languageDisplay(){ throw new Error("[[Get]] languageDisplay") }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see tc39/proposal-intl-displaynames-v2#34

4. Intl.DisplayNames ( locales, options )
Normal convention should still be to always validate all options, even when unused.
That means the GetOption(options, "languageDisplay", ...) step should happen before If type is "language", then. (But don't move assigning displayNames.[[LanguageDisplay]] outside of the if-step.)
For example compare to SetNumberFormatUnitOptions().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryzokuken
Copy link
Member

I think all outstanding comments have been addressed and this PR has the editorial signoffs needed? Merging.

@ryzokuken ryzokuken merged commit 5b84c6f into tc39:master Jan 12, 2022
tidoust added a commit to w3c/browser-specs that referenced this pull request Jan 25, 2022
Proposal reached stage 4, was integrated in Ecma402 and now returns a 404:
tc39/proposals@f4378e1
tc39/ecma402#622
tidoust added a commit to w3c/browser-specs that referenced this pull request Jan 25, 2022
* Drop Intl.displayNames v2 proposal, merged in Ecma402

Proposal reached stage 4, was integrated in Ecma402 and now returns a 404:
tc39/proposals@f4378e1
tc39/ecma402#622

* Drop Intl.Segmenter proposal, merged in Ecma402

Proposal moved to stage 4, and was merged in Ecma402:
tc39/proposals@24bdab7
tc39/ecma402#553
@FrankYFTang FrankYFTang deleted the DisplayNamesStage4PR branch July 25, 2023 05:19
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