Editorial: Define a structured signature for PluralRuleSelect #594

gibson042 · 2021-07-15T19:50:25Z

Fixes #593

gibson042 · 2021-07-15T19:51:03Z

spec/datetimeformat.html

@@ -374,7 +374,7 @@ <h1>FormatDateTimePattern ( _dateTimeFormat_, _patternParts_, _x_, _rangeFormatO
 1. Else if _p_ is equal to *"timeZoneName"*, then
 1. Let _f_ be _dateTimeFormat_.[[TimeZoneName]].
 1. Let _v_ be _dateTimeFormat_.[[TimeZone]].
- 1. Let _fv_ be a String value representing _v_ in the form given by _f_; the String value depends upon the implementation and the effective locale. The String value may also depend on the value of the [[InDST]] field of _tm_.  If the implementation does not have a localized representation of _f_, then use the String value of _v_ itself.
+ 1. Let _fv_ be a String value representing _v_ in the form given by _f_; the String value depends upon the implementation and the effective locale. The String value may also depend on the value of the [[InDST]] field of _tm_. If the implementation does not have a localized representation of _f_, then use the String value of _v_ itself.


This removal of consecutive whitespace resolves a warning from ecmarkup.

gibson042 · 2021-07-15T19:52:14Z

spec/pluralrules.html

+ <emu-clause id="sec-pluralruleselect" type="implementation-defined abstract operation">
+ <h1>
+ PluralRuleSelect (
+ _locale_: a String,
+ _type_: a String,
+ _n_: a finite Number,
+ _operands_: a Plural Rules Operands Record corresponding with _n_,
+ )
+ </h1>
+ <dl class="header">


Further adoption of structured operation headers can be accomplished later.

I assume this is a new convention from ECMA-262? It looks great, thanks!

gibson042 · 2021-07-15T20:05:06Z

spec/pluralrules.html

+ </h1>
+ <dl class="header">
+ <dt>description</dt>
+ <dd>It returns the String from &laquo; *"zero"*, *"one"*, *"two"*, *"few"*, *"many"*, *"other"* &raquo; that best categorizes _n_ according to the rules for _locale_ and _type_.</dd>


Note that operands doesn't actually seem relevant to the signature of PluralRuleSelect... I'd like to take this PR even further, removing locale, type, and operands and replacing them with a single pluralRules argument (an instance with an [[InitializedPluralRules]] slot) for better comprehensibility and alignment with other parts of the spec, in the process also removing GetOperands as an implementation detail (since its only use is to construct operands).

sffc

Thanks! Once this is merged I'll pull it in to NFv3.

ryzokuken

LGTM. Not sure if the follow-on change you mentioned will keep this PR editorial or not... @sffc @zbraniecki what do you think?

ryzokuken · 2021-07-22T04:08:41Z

spec/pluralrules.html

- <p>
- When the PluralRuleSelect abstract operation is called with four arguments, it performs an implementation-dependent algorithm to map _n_ to the appropriate plural representation of the Plural Rules Operands Record _operands_ by selecting the rules denoted by _type_ for the corresponding _locale_, or the String value *"other"*.
- </p>
+ <emu-clause id="sec-pluralruleselect" type="implementation-defined abstract operation">


The aoid is gone, I suppose automagic linking of this AO from algorithms won't work anymore.

aoid is now pulled from the structured header, cf. https://github.com/tc39/ecmarkup/blob/6253fe32dec8a8b7f4271e76f734fbab2ee4c5f4/src/Clause.ts#L122-L143

oh wow, this is great. I should really keep up with all the exciting new stuff in ecmarkup

ryzokuken · 2021-07-22T04:10:35Z

spec/pluralrules.html

+ <emu-clause id="sec-pluralruleselect" type="implementation-defined abstract operation">
+ <h1>
+ PluralRuleSelect (
+ _locale_: a String,
+ _type_: a String,
+ _n_: a finite Number,
+ _operands_: a Plural Rules Operands Record corresponding with _n_,
+ )
+ </h1>
+ <dl class="header">


I assume this is a new convention from ECMA-262? It looks great, thanks!

gibson042 · 2021-07-22T05:00:11Z

Not sure if the follow-on change you mentioned [removing locale, type, and operands and replacing them with a single pluralRules argument (an instance with an [[InitializedPluralRules]] slot) for better comprehensibility and alignment with other parts of the spec, in the process also removing GetOperands as an implementation detail] will keep this PR editorial or not... @sffc @zbraniecki what do you think?

I believe it would because PluralRuleSelect is called only from ResolvePlural, which pulls locale and type directly from slots on pluralRules and determines operands by invoking GetOperands on a value from the response of FormatNumericToString(pluralRules, n). So if PluralRuleSelect were provided with pluralRules and n similar to e.g. FormatDateTime(dateTimeFormat, x) and FormatNumeric(numberFormat, x), then it could do those steps itself (reading [[Locale]] and [[Type]] and determining how they affect the result of "zero" vs. "one" vs. "two" vs. "few" vs. "many" vs. "other").

Unless there's some reason why the internal behavior of plural rule selection MUST format the number to a numeric string and then use the resulting integer/fraction digit counts to make its determination, even though that determination is itself implementation-defined?

anba · 2021-07-22T07:26:30Z

The number is formatted as a string, because plural rules make a difference between integers and non-integers:

js> new Intl.PluralRules("en", {minimumFractionDigits: 0}).select(1) 
"one"
js> new Intl.PluralRules("en", {minimumFractionDigits: 2}).select(1) 
"other"

And by specifying that FormatNumericToString and GetOperands must be used, we can control how the input number is formatted, which gives less wiggle room for an implementation to do the wrong thing.

gibson042 · 2021-07-22T13:51:24Z

I just don't see how that's the case... non-normative references to CLDR and in-spec examples would help much more than requiring a roundabout trip from Number to String to Record—especially when the Record appears to be an attempt to recover integer/fraction/significant digits configuration that already exists on pluralRules. The current algorithm seems overly prescriptive, and rather divergent from other parts of the spec.

Maybe what's putting me off is that PluralRuleSelect has access to n in addition to the Record derived from inspecting its string serialization subject to pluralRules, but does not have access to pluralRules itself... it seems so strange to provide both n and operands.

gibson042 · 2021-07-22T17:38:25Z

spec/pluralrules.html

@@ -79,35 +75,42 @@ <h1>GetOperands ( _s_ )</h1>
 <td>Number of digits of [[Number]].</td>


@sffc there are problems with the definitions of these fields:

[[IntegerDigits]] is defined as "Number of digits of [[Number]]", but is populated with the number value of the integer digits (e.g., it is equal to 8 for an input string like "8.540").

[[FractionDigits]] and [[NumberOfFractionDigits]] have effectively the same definition, even though the former is populated with the number value of the fractional digits while the latter is populated with the count of such digits (e.g., they are respectively equal to 540 and 3 for an input string like "8.540").

[[FractionDigitsWithoutTrailing]] and [[NumberOfFractionDigitsWithoutTrailing]] also have effectively the same definition, even though the former is populated with the number value of the truncated fractional digits while the latter is populated with the count of such digits (e.g., they are respectively equal to 54 and 2 for an input string like "8.540").

I'd like to fix this... do you know what the actual intent is?

I think these are supposed to correspond to: https://unicode.org/reports/tr35/tr35-numbers.html#Operands

i | integer digits of n.

v | number of visible fraction digits in n, with trailing zeros.*

w | number of visible fraction digits in n, without trailing zeros.*

f | visible fraction digits in n, with trailing zeros.*

t | visible fraction digits in n, without trailing zeros.*

Thanks. I added references and updated the text to match (which is still just editorial work).

gibson042 · 2021-07-22T17:55:34Z

Maybe what's putting me off is that PluralRuleSelect has access to n in addition to the Record derived from inspecting its string serialization subject to pluralRules, but does not have access to pluralRules itself... it seems so strange to provide both n and operands.

Concrete proposal: drop GetOperands and change the signature of PluralRuleSelect to accept a String ([[FormattedString]] output from FormatNumericToString) rather than a Number and a Record. The string parsing of GetOperands would become an implementation detail, but the implementation-defined behavior would not have access to the raw number.

         1. Let _res_ be ! FormatNumericToString(_pluralRules_, _n_).
-        1. Let _s_ be _res_.[[FormattedString]].
-        1. Let _operands_ be ! GetOperands(_s_).
-        1. Return ! PluralRuleSelect(_locale_, _type_, _n_, _operands_).
+        1. Return ! PluralRuleSelect(_locale_, _type_, _res_.[[FormattedString]]).

Fixes tc39#593

spec/pluralrules.html

gibson042 requested a review from sffc July 15, 2021 19:50

gibson042 commented Jul 15, 2021

View reviewed changes

sffc requested review from zbraniecki and ryzokuken July 22, 2021 00:01

sffc approved these changes Jul 22, 2021

View reviewed changes

ryzokuken approved these changes Jul 22, 2021

View reviewed changes

ryzokuken changed the title ~~Define a structured signature for PluralRuleSelect~~ Editorial: Define a structured signature for PluralRuleSelect Jul 22, 2021

ryzokuken added the editorial Involves an editorial fix label Jul 22, 2021

gibson042 commented Jul 22, 2021

View reviewed changes

gibson042 force-pushed the gh-593-improve-PluralRuleSelect branch 3 times, most recently from 299e92c to a93d608 Compare July 23, 2021 02:58

gibson042 added 5 commits July 22, 2021 23:22

Meta: Upgrade ecmarkup

5f843cb

Editorial: Define a structured signature for PluralRuleSelect

0624298

Fixes tc39#593

Editorial: Restore use of _operands_ in PluralRuleSelect

1b78e65

Editorial: Define a structured signature for GetOperands

a7c7fc2

Editorial: Align GetOperands with UTS tc39#35

b8993d2

gibson042 force-pushed the gh-593-improve-PluralRuleSelect branch from a93d608 to b8993d2 Compare July 23, 2021 03:24

zbraniecki approved these changes Aug 12, 2021

View reviewed changes

gibson042 mentioned this pull request Aug 15, 2021

Meta: Upgrade ecmarkup #600

Merged

Merge branch 'master' into tc39gh-593-improve-PluralRuleSelect

7024f5f

gibson042 commented Jan 6, 2022

View reviewed changes

spec/pluralrules.html Outdated Show resolved Hide resolved

Editorial: Use conventional string substring phrasing

d440499

gibson042 merged commit 06cfbb9 into tc39:master Jan 6, 2022

gibson042 mentioned this pull request Jan 6, 2022

Inline GetOperands into PluralRuleSelect #635

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: Define a structured signature for PluralRuleSelect #594

Editorial: Define a structured signature for PluralRuleSelect #594

gibson042 commented Jul 15, 2021

gibson042 Jul 15, 2021

gibson042 Jul 15, 2021

ryzokuken Jul 22, 2021

gibson042 Jul 15, 2021 •

edited

Loading

sffc left a comment

ryzokuken left a comment

ryzokuken Jul 22, 2021

gibson042 Jul 22, 2021

ryzokuken Jul 22, 2021

ryzokuken Jul 22, 2021

gibson042 commented Jul 22, 2021 •

edited

Loading

anba commented Jul 22, 2021

gibson042 commented Jul 22, 2021 •

edited

Loading

gibson042 Jul 22, 2021

sffc Jul 22, 2021

gibson042 Jul 23, 2021

gibson042 commented Jul 22, 2021 •

edited

Loading

		@@ -79,35 +75,42 @@ <h1>GetOperands ( _s_ )</h1>
		<td>Number of digits of [[Number]].</td>

Editorial: Define a structured signature for PluralRuleSelect #594

Editorial: Define a structured signature for PluralRuleSelect #594

Conversation

gibson042 commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

ryzokuken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Jul 22, 2021 • edited Loading

anba commented Jul 22, 2021

gibson042 commented Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Jul 22, 2021 • edited Loading

gibson042 Jul 15, 2021 •

edited

Loading

gibson042 commented Jul 22, 2021 •

edited

Loading

gibson042 commented Jul 22, 2021 •

edited

Loading

gibson042 commented Jul 22, 2021 •

edited

Loading