Conversation
|
gibson042
left a comment
There was a problem hiding this comment.
ApplyDigitOptionsis essentially just 402'sFormatNumericToStringwithout referrring to some internal slots in Intl.NF.
Does that mean that FormatNumericToString can be simplified to leverage ApplyDigitOptions?
spec.emu
Outdated
| <emu-clause id="sec-amount-getunitconversionfactor" type="abstract operation"> | ||
| <h1>GetUnitConversionFactor ( | ||
| _unit_: a String | ||
| ): either a normal completion containing a Record with fields [[Category]] (a String), [[Factor]] (a Number), and [[Offset]] (a Number) or a throw completion | ||
| </h1> | ||
| <dl class="header"> | ||
| <dt>description</dt> | ||
| <dd>It returns the conversion data for converting _unit_ to its base unit within its unit category.</dd> | ||
| </dl> | ||
| <emu-alg> | ||
| 1. If _unit_ is not recognized as a unit for which conversion data is available, throw a *RangeError* exception. | ||
| 1. Return an implementation-defined Record { [[Category]], [[Factor]], [[Offset]] } where [[Category]] is a String identifying the unit category of _unit_ (e.g., *"length"*, *"mass"*, *"temperature"*), [[Factor]] is a Number representing the multiplication factor for converting from _unit_ to the base unit of its category, and [[Offset]] is a Number representing the additive offset for converting from _unit_ to the base unit (*+0*<sub>𝔽</sub> for most units; non-zero for temperature units such as *"fahrenheit"*). | ||
| </emu-alg> | ||
| <emu-note> | ||
| <p>Implementations should provide conversion data consistent with the Unicode Common Locale Data Repository (CLDR) <code>units.xml</code> supplemental data. The formula for converting a value in _unit_ to its base unit is: <i>baseValue</i> = <i>value</i> × [[Factor]] + [[Offset]].</p> | ||
| <p>CLDR expresses conversion factors as rational numbers (e.g., 0.3048/12 for inch-to-meter). These rational values are converted to Numbers before use, so conversion arithmetic is subject to the precision of IEEE 754 binary64. For example, converting 1.75 feet to inches yields 1.75 × 0.3048 / (0.3048 / 12) = 20.999999999999996, not exactly 21.</p> | ||
| </emu-note> | ||
| </emu-clause> |
There was a problem hiding this comment.
I was expecting a normative reference to CLDR file units.xml, not a suggestion that allows for arbitrary implementation divergence. This description isn't even appropriate for type="abstract operation"; it would need type="implementation-defined abstract operation".
There was a problem hiding this comment.
I saw the implementation-defined abstract operation suggestions, thanks! This is my honest best guess for how to normatively refer to CLDR. At a minimum, I suppose we ought to add CDLR to the Bibliography. But as for the verbiage for normatively referring, I didn't quite know what to say except this. Any advice is welcome.
There was a problem hiding this comment.
The spec enumerates Normative References, and sample use thereof can be seen in e.g. AvailableNamedTimeZoneIdentifiers.
intl.emu
Outdated
| 1. Let _region_ be the region subtag of the result of applying the <emu-xref href="#sec-canonicalizelocalelist">CanonicalizeLocaleList</emu-xref> algorithm to _locale_, or the likely region if no region subtag is present. | ||
| 1. Let _preferredUnit_ be the implementation-defined preferred unit for _category_, _region_, and _usage_ based on CLDR unit preferences data. |
There was a problem hiding this comment.
I wasn't quite sure what else to write here; the handwaviness isn't intentional. I guess one step forward would be to say what types of values these are (String, of course), but I'm open to suggestions.
There was a problem hiding this comment.
For starters, CanonicalizeLocaleList doesn't make sense if we already have a single string, although I think we probably do want to support amount.convertTo({ locale: ["zh-hant", "zh-hans"] }) just like number.toLocaleString(["zh-hant", "zh-hans"]), and presumably also Unicode extension subtags "ms" (Measurement system) and "mu" (Measurement unit override). I think we'll want to go through ResolveLocale, possibly via ResolveOptions, and in either case that means this algorithm must provide available locales and their corresponding data.
There was a problem hiding this comment.
That sounds great. I had a multi-locale approach in mind, but the issue with measurement systems and overrides is a bit new to me and I'm hesitant to unilaterally add spec text; I'd like to discuss this in the JS numerics call first. I agree that we'll want to go through ResolveLocale/ResolveOptions, but I wonder what kind of extension to 402 we're talking about here. cc @ben-allen
There was a problem hiding this comment.
We now resolve locales in the established string-or-array-of-strings way, but for now I'm punting on the issue of resolving things like measurement systems and unit overrides.
intl.emu
Outdated
| </dl> | ||
| <emu-alg> | ||
| 1. <ins>If _value_ has the [[Value]] and [[FractionDigits]] internal slots, let _primValue_ be RenderAmountValueWithFractionDigits(_value_.[[Value]], _value_.[[FractionDigits]]) else</ins><del>Let</del> <ins>let</ins _primValue_ be ? ToPrimitive(_value_, ~number~). | ||
| 1. <ins>If _value_ has the [[Value]] internal slot, let _primValue_ be _value_.[[Value]]; else</ins> <del>Let</del> <ins>let</ins> _primValue_ be ? ToPrimitive(_value_, ~number~). |
There was a problem hiding this comment.
This is dangerous; even if there's not currently any userspace-exposed object with a [[Value]] slot other than an Amount instance, that could change at any time. The spec needs a better means of brand-checking than this.
eemeli
left a comment
There was a problem hiding this comment.
Sorry, I'm not finding time atm to do a complete review, so submitting just a few comments for now.
Beyond the currently-changed lines, we should leave RS: StringIntlMV as a TODO, given how it's building on top of keep-trailing-zeros, and the implementation that's here is already known to be missing details like the accurate handling of inputs with negative exponents.
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>
Yeah that sounds good; that's what I was trying to hint at. I suppose one way to proceed would be to (1) do the refactoring in 402, then (2) copy-and-paste that newly refactored AO ( |
Prevent any kind of mixing of Amounts with other values, e.g., via mathematical operators.
82da3e7 to
f83d429
Compare
sffc
left a comment
There was a problem hiding this comment.
Mostly good; I don't like taking Intl.NF digit options spec and putting it in 262 but I guess that what we need to do in some form if we are going to be using those same options in Amount. We should discuss that further. But it doesn't block stage 2.
spec.emu
Outdated
| </dl> | ||
| <emu-alg> | ||
| 1. If _unit_ is not recognized as a unit for which conversion data is available, throw a *RangeError* exception. | ||
| 1. Return an implementation-defined Record { [[Category]], [[Factor]], [[Offset]] } where [[Category]] is a String identifying the unit category of _unit_ (e.g., *"length"*, *"mass"*, *"temperature"*), [[Factor]] is a Number representing the multiplication factor for converting from _unit_ to the base unit of its category, and [[Offset]] is a Number representing the additive offset for converting from _unit_ to the base unit (*+0*<sub>𝔽</sub> for most units; non-zero for temperature units such as *"fahrenheit"*). |
There was a problem hiding this comment.
Note: there are also nonlinear conversions such as the Richter scale and Beaufort scale.
There was a problem hiding this comment.
How should we handle these cases? One approach would be to have a converters that specify how to go to and from units (I'm thinking of a Record that would have toUnit and fromUnit keys whose values are mathematical functions). This would be inching us towards a "symbolic" approach with rational numbers that we discussed earlier.
There was a problem hiding this comment.
Richter isn't supported by Intl.NumberFormat or CLDR, and while Beaufort is supported by CLDR/ICU, it's not supported by Intl.NumberFormat. It's also the only entry with a special rule.
I don't think we should initially aim to support special conversions.
spec.emu
Outdated
| 1. Return an implementation-defined Record { [[Category]], [[Factor]], [[Offset]] } where [[Category]] is a String identifying the unit category of _unit_ (e.g., *"length"*, *"mass"*, *"temperature"*), [[Factor]] is a Number representing the multiplication factor for converting from _unit_ to the base unit of its category, and [[Offset]] is a Number representing the additive offset for converting from _unit_ to the base unit (*+0*<sub>𝔽</sub> for most units; non-zero for temperature units such as *"fahrenheit"*). | ||
| </emu-alg> | ||
| <emu-note> | ||
| <p>Implementations should provide conversion data consistent with the Unicode Common Locale Data Repository (CLDR) <code>units.xml</code> supplemental data. The formula for converting a value in _unit_ to its base unit is: <i>baseValue</i> = <i>value</i> × [[Factor]] + [[Offset]].</p> |
There was a problem hiding this comment.
Do we want to actually normatively reference units.xml? What benefit do we get from that versus some other independent researcher's data that conforms to the same contract?
There was a problem hiding this comment.
I was also a bit uncomfortable referencing units.xml. The only alternative I can see is to hardcode (some of) the data into 262.
There was a problem hiding this comment.
I think we do want to normatively refer to units.xml. See #80 (comment) to prior art for such Unicode references.
There was a problem hiding this comment.
Do we want to actually normatively reference units.xml? What benefit do we get from that versus some other independent researcher's data that conforms to the same contract?
An objective artifact against which nonconformance can be definitively identified. There is no research to be done regarding e.g. the ratio between meters and feet.
spec.emu
Outdated
| </emu-clause> | ||
|
|
||
| <emu-clause id="sec-amount-getconverttodigitoptions" type="abstract operation"> | ||
| <h1>GetConvertToDigitOptions ( |
There was a problem hiding this comment.
Thought: I'm not happy with GetNumberFormatDigitOptions spec text bleeding into other parts of the spec. This AO is not one to be proud of.
There was a problem hiding this comment.
I've gone a step further and removed those AOs entirely. It seems to me that the min/max digit stuff is a display concern that probably belongs in Intl.NumberFormat or a 402-specific override of .convertTo. I've tweaked things so that both the 262 and 402 .convertTo now use the same approach to precision as the constructor (the fractionDigits, significantDigits, and rounding options).
wdyt @eemeli ? Where should the min/max fraction/significant digit stuff go? I feel this conflicts with what you sketched in #80 . Maybe I'm misunderstanding something.
There was a problem hiding this comment.
Min/max fraction/significant digit handling during conversion should end up in 262, as explicit-unit conversion is ending up in 262, and the rounding issues that are addressed by those options and the default behaviour around them are not 402 problems.
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Delete GetConvertToDigitOptions and ApplyDigitOptions; both convertTo variants now use GetAmountOptions (fractionDigits/significantDigits/roundingMode).
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
The [[Value]] internal slot now stores the original JavaScript value type (a disjunction of Number, BigInt, or String) rather than always a mathematical value. When precision options are applied or unit conversion is performed, the value is stored as a decimal digit string. The [[FractionDigits]] internal slot has been removed. The
.fractionDigits,.significantDigits, and.with()are removed; a.valueaccessor is added that returns [[Value]] directly. We also now refer to CLDR's units data.There's a a new
Amount.prototype.convertTo()for unit conversion. 402 overrides this to add support for locale- and usage-based conversion via CLDR's unit preferences.New AOs:
GetUnitConversionFactor: Returns the CLDR-derived scalar factor and offset for converting a unit to its category's base unitConvertUnitValue: Converts a Number value between two units via their shared base unit, using Number arithmeticGetConvertToDigitOptions: Reads Intl.NumberFormat-style digit options from an options bagApplyDigitOptions: Rounds a Number per digit options and returns a StringResolveUnitPreference: Resolves a target unit from locale and usage via CLDR unit preferences (402-only)ApplyDigitOptionsis essentially just 402'sFormatNumericToStringwithout referrring to some internal slots in Intl.NF.