Editorial: Refactor handling of numeric-like styles, support differing hours/minutes vs minutes/seconds separators #188

ben-allen · 2024-02-22T14:43:25Z

Refactors handling of "numeric"-like styles
Add additional slots to handle 2-digit hours in locales that use them in all cases
Add additional slots to handle differing hours/minutes separators in some locales

Fixes #161

ryzokuken

Massive improvement, kudos! A bunch of comments inline to make this PR perfect :D

ryzokuken · 2024-02-22T14:52:24Z

spec.emu

+      <h1>
+        NextUnitFractional (
+          _durationFormat_: a DurationFormat Object,
+          _duration_: a Duration Record,


duration is never used in this AO, you can drop the argument.

ryzokuken · 2024-02-22T14:58:33Z

spec.emu

@@ -1,4 +1,5 @@
 <!doctype html>
+              1. Perform SetFractionalDigitOptions(_durationFormat_, _nfOpts_).


Suggested change

1. Perform SetFractionalDigitOptions(_durationFormat_, _nfOpts_).

ryzokuken · 2024-02-22T15:02:13Z

spec.emu

          1. If _unit_ is *"minutes"* or *"seconds"*, then
            1. Set _style_ to *"2-digit"*.
+        1. If _unit_ is *"hours"* and _durationFormat_.[[TwoDigitHours]] is *true*, then
+          1. Set _style_ to *"2-digit"*.


nit: it could be merged with the condition above

Suggested change

1. If _unit_ is *"minutes"* or *"seconds"*, then

1. Set _style_ to *"2-digit"*.

1. If _unit_ is *"hours"* and _durationFormat_.[[TwoDigitHours]] is *true*, then

1. Set _style_ to *"2-digit"*.

1. If _unit_ is *"minutes"* or *"seconds"* or _unit_ is *"hours"* and _durationFormat_.[[TwoDigitHours]] is *true*, then

1. Set _style_ to *"2-digit"*.

My thought is to leave this one as-is, because stringing together four conditionals on one line will likely be annoying for implementers

ryzokuken · 2024-02-22T15:04:52Z

spec.emu

@@ -495,18 +697,27 @@ contributors: Ujjwal Sharma, Younies Mahmoud
        1. Let _r_ be ResolveLocale(%DurationFormat%.[[AvailableLocales]], _requestedLocales_, _opt_, %DurationFormat%.[[RelevantExtensionKeys]], %DurationFormat%.[[LocaleData]]).
        1. Let _locale_ be r.[[locale]].
        1. Set _durationFormat_.[[Locale]] to _locale_.
+        1. Set _durationFormat_.[[DataLocale]] to _r_.[[dataLocale]].
+        1. Let _dataLocale_ be _durationFormat_.[[DataLocale]].
+        1. Let _dataLocaleData_ be _durationFormat_.[[LocaleData]].[[&lt;_dataLocale_&gt;]].


This doesn't look right?

Also you no longer need dataLocaleData IIUC, you can directly extract the value of digitalFormat and set the values in the instance from that.

going to leave this one for now -- I'm working on a PR to update DurationFormat to reflect changes made in tc39/ecma402#849

ryzokuken · 2024-02-22T15:08:50Z

spec.emu

+
+    <emu-clause id="sec-listformatparts" type="abstract operation">
+      <h1>ListFormatParts (
+          _nfResult_: a List of Records,


I'd be in favor of generalizing this AO by renaming things from nfResultFoo which is specific to our use case (we only list format the results of NF) to something more general.

ryzokuken · 2024-02-22T15:11:30Z

spec.emu

+              1. If _durationFormat_.[[FractionalDigits]] is *undefined*, then
+                1. Let _maximumFractionDigits_ be *9*<sub>𝔽</sub>.
+                1. Let _minimumFractionDigits_ be *+0*<sub>𝔽</sub>.
+              1. Else,
+                1. Let _maximumFractionDigits_ be _durationFormat_.[[FractionalDigits]].
+                1. Let _minimumFractionDigits_ be _durationFormat_.[[FractionalDigits]].
+              1. Perform ! CreateDataPropertyOrThrow(_nfOpts_, *"maximumFractionDigits"*, _maximumFractionDigits_).
+              1. Perform ! CreateDataPropertyOrThrow(_nfOpts_, *"minimumFractionDigits"*, _minimumFractionDigits_).
+              1. Perform ! CreateDataPropertyOrThrow(_nfOpts_, *"roundingMode"*, *"trunc"*).


Didn't you move this into an AO? Idk how it ended up back in.

So here's my thoughts on this: I suspect implementers might be annoyed by having the properties on nfOpts set in multiple AOs, since after a cursory glance I didn't find anything that does that in either 402 or Temporal.

@FrankYFTang @gibson042

ryzokuken · 2024-02-22T15:15:03Z

spec.emu

+        1. Let _secondsValue_ be _duration_.[[Seconds]].
+        1. Let _fractionalDigits_ be AddFractionalDigits(_durationFormat_, _duration).
+        1. If _fractionalDigits_ is not 0, then
+          1. If _durationFormat_.[[FractionalDigits]] is *undefined*, then


Replace with SetFractionalDigitOptions

fwiw I don't think it's particularly confusing for implementers if some options are set in an AO as long as it results in cleaner/DRY-er spec text... perhaps implementers can give their thoughts on the subject.

ryzokuken · 2024-02-22T15:15:25Z

spec.emu

+          1. If _unit_ is *"seconds"*, *"milliseconds"*, or *"microseconds"*, then
+            1. If NextUnitFractional(_durationFormat_, _duration_, _unit_) is *true*, then
+              1. Set _value_ to _value_ + AddFractionalDigits(_durationFormat_, _duration_).
+              1. If _durationFormat_.[[FractionalDigits]] is *undefined*, then


Replace with SetFractionalDigitOptions

ben-allen · 2024-02-22T18:41:53Z

Supersedes #186

sffc · 2024-02-23T23:15:43Z

TG2 discussion: https://github.com/tc39/ecma402/blob/main/meetings/notes-2024-02-22.md#durationformat-update

FrankYFTang · 2024-02-24T00:03:07Z

spec.emu

@@ -189,6 +190,7 @@ contributors: Ujjwal Sharma, Younies Mahmoud
    <emu-clause id="sec-getdurationunitoptions" type="abstract operation">
      <h1>
        GetDurationUnitOptions (
+          _durationFormat_: a DurationFormat Object,


Why do we need to pass in a DurationFormat Object here? I see you only use durationFormat.[[TwoDigitHours]] inside this AO. Why not just pass in that value only ?

FrankYFTang · 2024-02-24T00:05:32Z

spec.emu

-        PartitionDurationFormatPattern (
-          _durationFormat_: a DurationFormat,
+        SetFractionalDigitOptions (
+        _durationFormat_: a DurationFormat Object,


The only thing you use internally is durationFormat.[[FractionalDigits]], why not just pass in that value and avoid passing in durationFormat ?

FrankYFTang · 2024-02-24T00:07:49Z

spec.emu

+    <emu-clause id="sec-formathours" type="abstract operation">
+      <h1>
+        FormatHours (
+          _durationFormat_: a DurationFormat Object,


how about we only pass in durationFormat.[[Locale]] and durationFormat.[[HoursStyle]] but not passing in durationFormat?

ben-allen · 2024-03-05T23:03:12Z

Incorporated feedback, made a few additional changes:

renamed isFirstUnitDisplayed and isSignDisplayed aliases to signDisplayed throughout
Renamed FormatHours, FormatMinutes, FormatSeconds to FormatNumericHours, FormatNumericMinutes, FormatNumericSeconds to clarify usage
Removed SetFractionalDigitOptions

Justification for removing SetFractionalDigitOptions: Talked it over with @ptomato, who observed that it could be confusing to add nfOpts properties in multiple AOs due to how setting properties can trigger setters in user code. Options as I see it:

Leave as in this version of PR, with the steps for setting fractional digit options repeated in two places.
Restore SetFractionalDigitOptions, since we're passing *null* to OrdinaryObjectCreate when creating nfOpts objects and so, if I understand it correctly, the poisoned object problem won't actually occur. However: creating properties on objects across multiple AOs is not currently something that happens in either 402 or Temporal
Rewrite SetFractionalDigitOptions to return a Record which is then used by the caller to create nfOpts properties. Note: this doesn't actually save that many repeated steps.

requesting re-review! @ryzokuken @FrankYFTang

ben-allen · 2024-04-12T01:46:20Z

Requesting re-review! @FrankYFTang @ryzokuken

ryzokuken

Overall LGTM, the improvements to the spec are quite evident by the amount of time it took me to review it in entirety this time... Either that or there's something with my coffee :P

Anyway, I have made a flurry of comments that should all further clean up the text. All of this is very much editorial and fills me with hope that by the end of this exercise we'd have a pretty looking and readable spec.

ryzokuken · 2024-04-18T13:02:35Z

spec.emu

@@ -234,6 +235,8 @@ contributors: Ujjwal Sharma, Younies Mahmoud
            1. Throw a *RangeError* exception.
          1. If _unit_ is *"minutes"* or *"seconds"*, then
            1. Set _style_ to *"2-digit"*.
+        1. If _unit_ is *"hours"* and _twoDigitHours_ is *true*, then


I tried to think about this for a while but I don't feel like I can approve this. We need to think of another approach because overriding the hours style like this at the end can be extremely surprising to the caller.

We've had the previous lines about forcing minutes and seconds to "2-digit" when preceded by a "numeric" or "2-digit" style in the spec for quite some time, and I view this as analogous. I'm working on the assumption -- which may be incorrect -- that in the locales that always use a two-digit representation of hours, displaying a one-digit hour is analogous to using the wrong separator.

ryzokuken · 2024-04-18T13:07:16Z

spec.emu

+        1. If _unit_ is *"seconds"*, then
+          1. If _durationFormat_.[[MillisecondsStyle]] is *"fractional"*, return *true*.
+        1. If _unit_ is *"milliseconds"*, then
+          1. If _durationFormat_.[[MicrosecondsStyle]] is *"fractional"*, return *true*.
+        1. If _unit_ is *"microseconds"*, then
+          1. If _durationFormat_.[[NanosecondsStyle]] is *"fractional"*, return *true*.


editorial nit but it'll be cleaner/shorter if you merge the conditions, atleast pair them up.

ryzokuken · 2024-04-18T13:08:27Z

spec.emu

+      </emu-alg>
+    </emu-clause>
+
+    <emu-clause id="sec-formathours" type="abstract operation">


id is "incorrect" is there a strong reason to exclude the word numeric from all these AOs?

ryzokuken · 2024-04-18T13:11:05Z

spec.emu

+        1. Let _nf_ be ! Construct(%NumberFormat%, &laquo; _durationFormat_.[[Locale]] &raquo;, _nfOpts_).
+        1. Let _hoursParts_ be ! PartitionNumberPattern(_nf_, _hoursValue_).
+        1. For each Record { [[Type]], [[Value]] } _part_ of _hoursParts_, do
+          1. Return the Record { [[Type]]: _part_.[[Type]], [[Value]]: _part_.[[Value]], [[Unit]]: *"hour"* }.


This is incorrect, you can't return in a loop like this. Construct a second list of records and return the whole list.

oof, can't believe I left that in

ryzokuken · 2024-04-18T13:16:22Z

spec.emu

+      </h1>
+      <dl class="header">
+        <dt>description</dt>
+        <dd>It creates a List of Records containing the formatted parts of the hours unit of _duration_ when the requested style is *"numeric"* or *"2-digit"* according to the effective locale and the formatting options of _durationFormat_.</dd>


Suggested change

<dd>It creates a List of Records containing the formatted parts of the hours unit of _duration_ when the requested style is *"numeric"* or *"2-digit"* according to the effective locale and the formatting options of _durationFormat_.</dd>

<dd>It creates a List of Records containing the formatted parts of the minutes unit of _duration_ when the requested style is *"numeric"* or *"2-digit"* according to the effective locale and the formatting options of _durationFormat_.</dd>

ryzokuken · 2024-04-18T13:48:49Z

spec.emu

+        1. Let _numericUnitFound_ be *false*.
+        1. While _numericUnitFound_ is *false*, repeat for each row in <emu-xref href="#table-partition-duration-format-pattern"></emu-xref> in table order, except the header row:
+          1. Let _value_ be the value of _duration_'s field whose name is the Value Field of the current row.
+          1. Let _style_ be the value of _durationFormat_'s internal slot whose name is the Style Slot value of the current row.


it's inconsistent with the last line, either say "value" in both or neither. Since it says "value" in the next line too, I guess you should just include it in the previous line.

ryzokuken · 2024-04-18T13:58:03Z

spec.emu

+                  1. Let _maximumFractionDigits_ be *9*<sub>𝔽</sub>.
+                  1. Let _minimumFractionDigits_ be *+0*<sub>𝔽</sub>.


I think these are the defaults anyway if we keep the values undefined?

I don't see where those values could get set -- isn't [[FractionDigits]] assigned a value returned from GetNumberOption with *undefined* as the default?

ryzokuken · 2024-04-18T14:03:01Z

spec.emu

+                1. Append the Record { [[Type]]: _part_.[[Type]], [[Value]]: _part_[[Value]], [[Unit]]: _numberFormatUnit_ } to _list_.
+              1. Append _list_ to _result_.
+        1. Return ListFormatParts(_durationFormat_, _result_).
+      </emu-alg>


Great! PDFP (if I can call it that) has become so much better!

ryzokuken · 2024-04-18T14:05:31Z

spec.emu

+        1. Set _durationFormat_.[[DataLocale]] to _r_.[[dataLocale]].
+        1. Let _dataLocale_ be _durationFormat_.[[DataLocale]].
+        1. Let _dataLocaleData_ be _durationFormat_.[[LocaleData]].[[&lt;_dataLocale_&gt;]].
+        1. Let _digitalFormat_ be _dataLocaleData_.[[DigitalFormat]].


Basically this is the value you actually need, the few lines right before this one can be collapsed into this.

I've thought pretty hard about this one. Collapsing these lines together doesn't match rest-of-402 style, as I understand it. See:

steps 18 and 19 of Intl.DisplayNames constructor:

18. Let _resolvedLocaleData_ be _r_.[[LocaleData]]. 19. Let _types_ be _resolvedLocaleData_.[[types]].

with resolvedLocaleData never used after the [[types]] access.

Steps 14 and 15 of Intl.ListFormat constructor, resolvedLocaleData likewise not used after resolvedLocaleData.[[<type>]] access:

14. Let _resolvedLocaleData_ be _r_.[[LocaleData]]. 15. Let _dataLocaleTypes_ be _resolvedLocaleData_.[[<_type_>]].

First two steps of GetNumberFormatPattern AO in Intl.NumberFormat

1. Let _resolvedLocaleData_ be _numberFormat_.[[LocaleData]]. 2. Let _patterns_ be _resolvedLocaleData_.[[patterns]].

(among others)

Additionally, the pattern "Let alias be obj.[[X]].[[Y]].[[Z]]" is very rare in 402 algorithms; it shows up in one place in Intl.DateTimeFormat and nowhere else (https://tc39.es/ecma402/#sec-date-time-style-format)

Four-deep accesses (as would happen here if these were collapsed together) occur not at all in rest-of-402

ryzokuken · 2024-04-18T14:06:06Z

spec.emu

+        1. Let _twoDigitHours_ be _digitalFormat_.[[TwoDigitHours]].
+        1. Set _durationFormat_.[[TwoDigitHours]] to _twoDigitHours_.
+        1. Let _hoursMinutesSeparator_ be _digitalFormat_.[[HoursMinutesSeparator]].
+        1. Set _durationFormat_.[[HoursMinutesSeparator]] to _hoursMinutesSeparator_.
+        1. Let _minutesSecondsSeparator_ be _digitalFormat_.[[MinutesSecondsSeparator]].
+        1. Set _durationFormat_.[[MinutesSecondsSeparator]] to _minutesSecondsSeparator_.


No need to set three variables, all of these lines can be collapsed.

Two-deep accesses are pretty rare across rest-of-402 -- only really shows up in Intl.DateTimeFormat spec. My leaning is toward leaving this one this way.

ben-allen · 2024-04-24T23:15:26Z

I've pushed a major update reflecting feedback from @ryzokuken and incorporating some additional bugfixes.

ben-allen · 2024-04-24T23:49:30Z

New revision ready for review! Much more solid thanks to @ryzokuken's feedback, but could still use more eyes. @gibson042 @FrankYFTang @sffc @anba

sffc · 2024-05-03T03:58:08Z

TG2 notes: https://github.com/tc39/ecma402/blob/main/meetings/notes-2024-04-25.md#durationformat-update

sffc

I reviewed this in enough detail to say that I believe it covers the edge cases we had identified and overall makes the spec much easier to reason about and implement. Please merge :)

* Separates out handling for `"numeric"` and `"2-digit"` styles, referred to as "digital styles" in the rest of this commit message, in `FormatNumericUnits`, `FormatNumericHours`, `FormatNumericMinutes`, `FormatNumericSeconds`, and `NextUnitFractional` AOs * New AO `ListFormatParts` for portion of `PartitionDurationFormatPattern` concerned with ListFormatting the NumberFormatted parts * Bug fix: Incorporate better support for certain locales: - locales like `"af"` that when using digital styles always use 2-digit hours, i.e. "09:00" rather than "9:00". Added [[HoursDigit]] slot. - locales like `"fr-CA"` that use a separator between minutes and seconds that differs from the separator between hours and minutes, as in `"fr-CA"`'s use of 'h' and "min" as separators: 8 h 59 min 59 for 8 hours, 59 minutes, 59 seconds. Added [[HoursMinutesSeparator]] and [[MinutesSecondsSeparator]] slots.

ben-allen force-pushed the new-feb-refactor branch from a3e86be to 6be3bc7 Compare February 22, 2024 14:45

ryzokuken requested changes Feb 22, 2024

View reviewed changes

FrankYFTang self-assigned this Feb 22, 2024

ben-allen changed the title ~~New feb refactor~~ Editorial: Refactor handling of numeric-like styles, support differing hours/minutes vs minutes/seconds separators Feb 22, 2024

ben-allen mentioned this pull request Feb 22, 2024

Editorial: Refactor handling of numeric-like styles, support differing hours/minutes vs minutes/seconds separators #186

Closed

FrankYFTang reviewed Feb 24, 2024

View reviewed changes

ben-allen force-pushed the new-feb-refactor branch from 49d20ac to 9c35e0d Compare March 5, 2024 22:47

ben-allen mentioned this pull request Mar 7, 2024

Digital duration format should probably be more lenient with data #161

Closed

This was referenced Apr 11, 2024

test formatting when microseconds or nanoseconds are the first numeric-styled unit tc39/test262#4034

Merged

Update Intl.DurationFormat tests tc39/test262#3983

Merged

ben-allen requested a review from gibson042 April 11, 2024 20:15

ben-allen force-pushed the new-feb-refactor branch from bb34091 to f815788 Compare April 12, 2024 00:57

ben-allen mentioned this pull request Apr 17, 2024

Fix and simplify "displayRequired" computation in PartitionDurationFormatPattern #183

Open

ryzokuken requested changes Apr 18, 2024

View reviewed changes

ben-allen force-pushed the new-feb-refactor branch 2 times, most recently from 88029c1 to c6025ef Compare April 24, 2024 23:03

sffc approved these changes May 16, 2024

View reviewed changes

ben-allen force-pushed the new-feb-refactor branch from 6a2ec8c to abe658d Compare May 20, 2024 18:46

ben-allen force-pushed the new-feb-refactor branch from 07bdbfa to 4cbcc7d Compare May 20, 2024 20:02

fixup: [spec] npm run build

1af3325

ben-allen merged commit 835300f into tc39:main May 20, 2024

This was referenced May 21, 2024

Editorial: PartitionDurationFormatPattern accessing non-existent slots #191

Closed

Incorrect access of [[HoursValue]] and [[SecondsValue]] of durationFormat in PartitionDurationFormatPattern #190

Closed

		@@ -1,4 +1,5 @@
		<!doctype html>
		1. Perform SetFractionalDigitOptions(_durationFormat_, _nfOpts_).

	<dd>It creates a List of Records containing the formatted parts of the hours unit of _duration_ when the requested style is "numeric" or "2-digit" according to the effective locale and the formatting options of _durationFormat_.</dd>
	<dd>It creates a List of Records containing the formatted parts of the minutes unit of _duration_ when the requested style is "numeric" or "2-digit" according to the effective locale and the formatting options of _durationFormat_.</dd>

		1. Let _maximumFractionDigits_ be 9<sub>𝔽</sub>.
		1. Let _minimumFractionDigits_ be +0<sub>𝔽</sub>.

Editorial: Refactor handling of numeric-like styles, support differing hours/minutes vs minutes/seconds separators #188

Editorial: Refactor handling of numeric-like styles, support differing hours/minutes vs minutes/seconds separators #188

Conversation

ben-allen commented Feb 22, 2024 • edited

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

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

ben-allen Mar 5, 2024 • edited

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

ben-allen commented Feb 22, 2024

sffc commented Feb 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-allen commented Mar 5, 2024

ben-allen commented Apr 12, 2024

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

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

ben-allen Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-allen Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-allen commented Apr 24, 2024 • edited

ben-allen commented Apr 24, 2024

sffc commented May 3, 2024

sffc left a comment

Choose a reason for hiding this comment

ben-allen commented Feb 22, 2024 •

edited

ben-allen Mar 5, 2024 •

edited

ben-allen Apr 23, 2024 •

edited

ben-allen Apr 24, 2024 •

edited

ben-allen commented Apr 24, 2024 •

edited