Conversation
…r where necessary
…oralPartialDurationRecord
Also change `value` computation to reduce indentation.
…c" or "2-digit"
```js
new Intl.DurationFormat("en", {
hours: "2-digit",
});
```
throws `RangeError: invalid value long for option minutes` according to the
original text, because the `baseStyle` is `"long"`, so when the default
`style` for the missing `"minutes"` option is computed, we use `"long"`,
which is incompatible with the `prevStyle` of `"2-digit"`.
spec.emu
Outdated
| 1. Let _style_ be the current value of the _styleSlot_ slot of _durationFormat_. | ||
| 1. Let _nextStyle_ be the current value of the _nextStyleSlot_ slot of _durationFormat_. | ||
| 1. Let _style_ be _durationFormat_.[[<_styleSlot_>]]. | ||
| 1. Let _nextStyle_ be _durationFormat_[[<_nextStyleSlot_>]]. |
There was a problem hiding this comment.
This is missing a dot before [[ here and below. Later commits fix these errors.
ryzokuken
left a comment
ryzokuken
left a comment
There was a problem hiding this comment.
Thanks for all your amazing work! I see you're either a git add --patch fan or just really organized (it was really helpful). Either way, I left some comments, a subset of which might require some discussion.
spec.emu
Outdated
| 1. If _style_ is `"2-digit"`, then | ||
| 1. Perform ! CreateDataPropertyOrThrow(_nfOpts_, `"minimumIntegerDigits"`, `2`). | ||
| 1. If _value_ is *+0*<sub>𝔽</sub> or *-0*<sub>𝔽</sub> and _display_ is `"auto"`, then | ||
| 1. If _value_ is 0 and _display_ is `"auto"`, then |
There was a problem hiding this comment.
I suppose I keep falling in the JavaScript trap of "this will be freed once out of scope" 🤦
spec.emu
Outdated
| 1. Skip to the next iteration. | ||
| 1. Let _nf_ be ! Construct(%NumberFormat%, « _durationFormat_.[[Locale]], _nfOpts_ »). | ||
| 1. Let _num_ be ! FormatNumeric(_durationFormat_.[[NumberFormat]], 𝔽(_value_)). | ||
| 1. Let _num_ be ! FormatNumeric(_nf_, 𝔽(_value_)). |
There was a problem hiding this comment.
I suppose I missed this when I moved from a single (cached) numberformat instance to dynamic one based on the style.
| 1. Let _unit_ be the Unit value. | ||
| 1. Let _style_ be the current value of the _styleSlot_ slot of _durationFormat_. | ||
| 1. Let _nextStyle_ be the current value of the _nextStyleSlot_ slot of _durationFormat_. | ||
| 1. Let _style_ be _durationFormat_.[[<_styleSlot_>]]. |
There was a problem hiding this comment.
I did it this way previously, but @bakkot asked me to switch. I don't mind either, but let's make sure we're all onboard with this style (fwiw, it's commonly used across 402).
There was a problem hiding this comment.
Did I? I don't recall. Given that this will go in 402 I'm fine with matching the prevailing editorial conventions of 402.
| 1. If 𝔽(_v_) is not finite, return *false*. | ||
| 1. Let _valueSlot_ be the Value Slot value of the current row. | ||
| 1. Let _v_ be _record_.[[<_valueSlot_>]]. | ||
| 1. Assert: 𝔽(_v_) is finite. |
There was a problem hiding this comment.
Is this a normative change? Although I'm not sure if we should cater to infinite values...
There was a problem hiding this comment.
Duration records in Intl.DurationFormat are always created through ToDurationRecord, which rejects infinite values (through the IsIntegralNumber check). I only performed this change, because we can't currently have infinite values, so it seems better to me to use an assertion.
| 1. Let _numberingSystem_ be ? GetOption(_options_, *"numberingSystem"*, *"string"*, *undefined*, *undefined*). | ||
| 1. If _numberingSystem_ is not *undefined, then | ||
| 1. If _numberingSystem_ does not match the Unicode Locale Identifier `type` nonterminal, throw a `RangeError` exception. | ||
| 1. If _numberingSystem_ does not match the Unicode Locale Identifier `type` nonterminal, throw a *RangeError* exception. |
There was a problem hiding this comment.
Regarding this and the previous two commits, I believe I switched from asterisks to backticks on @rbuckton's advice. That said, I'd be happy to do either as long as we have a clear contender.
There was a problem hiding this comment.
The style changed at some point → tc39/ecma402#396, which links to other discussions and the corresponding PR for ECMA-262.
spec.emu
Outdated
| 1. Skip to the next iteration. | ||
| 1. Let _nf_ be ? Construct(%NumberFormat%, « _durationFormat_.[[Locale]], _nfOpts_ »). | ||
| 1. Let _num_ be ! FormatNumeric(_durationFormat_.[[NumberFormat]], _value_). | ||
| 1. Let _num_ be ! FormatNumeric(_durationFormat_.[[NumberFormat]], 𝔽(_value_)). |
There was a problem hiding this comment.
𝔽 will need to be removed after Intl.NumberFormat V3 lands. But this will lead to a different output for some inputs, so if we care about the exact same output, this line will need to be changed to 𝔽(ℝ(value)).
Or alternatively we could make Intl.DurationFormat dependent on Intl.NumberFormat V3.
I guess it's better to open a separate issue about the precision, because Temporal typically generates the precise result.
There was a problem hiding this comment.
#110 has a concrete example to show the possible precision loss.
3 participants
Various small fixes. I've left two TODO notes, because fixing them requires larger changes.