[4th Edition] NumberFormat.prototype.formatToParts() #79

stasm · 2016-02-19T00:21:47Z

Related Issue: #30

caridy · 2016-02-19T00:28:38Z

/cc @littledan, @stasm will get this ready for you to review it.

stasm · 2016-02-19T00:50:02Z

I'd like to do another pass tomorrow and I'll let you know when this is ready. I'll work on the polyfill implementation next week. A huge thank-you to @caridy for his help with this!

stasm · 2016-02-19T10:54:21Z

spec/numberformat.html

@@ -409,7 +473,7 @@

      <ul>
        <li>The array that is the value of the "nu" property of any locale property of [[localeData]] must not include the values "native", "traditio", or "finance".</li>
-        <li>[[localeData]][locale] must have a patterns property for all locale values. The value of this property must be an object, which must have properties with the names of the three number format styles: *"decimal"*, *"percent"*, and *"currency"*. Each of these properties in turn must be an object with the properties positivePattern and negativePattern. The value of these properties must be string values that contain a substring *"{number}"*; the values within the currency property must also contain a substring *"{currency}"*. The pattern strings must not contain any characters in the General Category “Number, decimal digit" as specified by the Unicode Standard.</li>
+        <li>[[localeData]][locale] must have a patterns property for all locale values. The value of this property must be an object, which must have properties with the names of the three number format styles: *"decimal"*, *"percent"*, and *"currency"*. Each of these properties in turn must be an object with the properties positivePattern and negativePattern. The value of these properties must be string values that must contain the substring *"{number}"* and may contain the substrings *"{minusSign}"*, and *"{plusSign}"*; the values within the percent property must also contain the substring *"{percentSign}"*; the values within the currency property must also contain the substring *"{currency}"*. The pattern strings must not contain any characters in the General Category “Number, decimal digit" as specified by the Unicode Standard.</li>


@caridy Should we use earmuffs here for property names? E.g. a *"patterns"* property.

stasm · 2016-02-19T12:24:06Z

I filed http://unicode.org/cldr/trac/ticket/9284 to unify the names of separator parts in CLDR.

stasm · 2016-02-25T21:47:35Z

I filed http://unicode.org/cldr/trac/ticket/9284 to unify the names of separator parts in CLDR.

…and CLDR people said changing the names would introduce too much incompatibility. I suggest we stick to the names currently present in CLDR.

I changed my PR as follows:

instead of groupSeparator and decimalSeparator we now have group and decimal,
the actual sequences of digits (previously in my PR called group and decimal) are called integer and fraction.

N.B. The name integer works out nice when useGrouping is false.

stasm · 2016-02-26T16:49:15Z

spec/numberformat.html

+        1. Let _length_ be the number of code units in _pattern_.
+        1. Repeat while _beginIndex_ is an integer index into _pattern_:
+          1. Set _endIndex_ to Call(*%StringProto_indexOf%*, _pattern_, *"}"*, _beginIndex_)
+          1. If _endIndex_ is *false*, throw new Error exception.


@caridy Should this read false or perhaps -1 since we're calling indexOf?

stasm · 2016-02-26T19:37:22Z

The polyfill is now ready: andyearnshaw/Intl.js#149

stasm · 2016-03-01T10:14:35Z

I rebased this PR on top of the current master. I fixed conflicts caused by #80.

littledan · 2016-03-18T18:50:46Z

spec/numberformat.html

+        1. Assert: Type(_nf_) is Object and _nf_ has an [[initializedNumberFormat]] internal slot whose value is *true*.
+        1. If _value_ is not provided, let _value_ be *undefined*.
+        1. Let _x_ be ? ToNumber(_value_).
+        1. Return ? FormatNumberToParts(_nf_, _x_).


I don't think you can just directly return the result of FormatNumberToParts to ECMAScript. FormatNumberToParts returns a List of Records. You have to convert the List of Records into an Array of Objects.

@littledan - this is fixed now

zbraniecki · 2016-03-29T22:38:25Z

@stasm - I created stasm#1 to address Dan's comments.

zbraniecki · 2016-05-06T01:55:44Z

spec/numberformat.html

+
+      <emu-alg>
+        1. Let _nf_ be *this* value.
+        1. Assert: Type(_nf_) is Object and _nf_ has an [[initializedNumberFormat]] internal slot whose value is *true*.


@caridy said we should not have Asserts here. Instead, we should throw - see https://tc39.github.io/ecma402/#sec-intl.datetimeformat.prototype.format for example

use assertion to signal that ecmascript is broken, use throw to signal users are doing something wrong.

caridy · 2016-05-12T21:02:53Z

@stasm I have identified few issues when trying to release the polyfill, and I have commented here. Also, I have updated the polyfill to align better with spec text, you can see the details here: andyearnshaw/Intl.js@ccd5a41

stasm · 2016-05-14T07:23:28Z

Thanks, @caridy, I'll make the fixes early next week.

stasm · 2016-05-19T14:50:26Z

@caridy Thank you for the feedback! I addressed your comments.

caridy · 2016-05-19T15:16:11Z

LGTM

caridy · 2016-06-09T22:41:13Z

@stasm we need some rebase here.

then we should wait for consensus at TC39 before merging this.

stasm · 2016-06-10T11:40:44Z

@caridy Would like me to also squash all the commits into a single one?

caridy · 2016-06-10T12:54:46Z

@stasm yes.

> const usd = Intl.NumberFormat('en', { style: 'currency', currency: 'USD' }); > usd.format(123456.789) '$123,456.79' > usd.formatToParts(123456.789) [ { type: 'currency', value: '$' }, { type: 'integer', value: '123' }, { type: 'group', value: ',' }, { type: 'integer', value: '456' }, { type: 'decimal', value: '.' }, { type: 'fraction', value: '79' } ] > const pc = Intl.NumberFormat('en', { style: 'percent', minimumFractionDigits: 2 }) > pc.format(-0.123456) '-12.35%' > pc.formatToParts(-0.123456) [ { type: 'minusSign', value: '-' }, { type: 'integer', value: '12' }, { type: 'decimal', value: '.' }, { type: 'fraction', value: '35' }, { type: 'literal', value: '%' } ]

stasm · 2016-06-10T13:49:15Z

Rebased and squashed. Thanks for all the help to get this ready.

jswalden · 2016-08-05T18:35:46Z

spec/numberformat.html

        1. Return _nf_.[[boundFormat]].
      </emu-alg>
    </emu-clause>

+    <emu-clause id="sec-intl.numberformat.prototype.formattoparts">
+      <h1>Intl.NumberFormat.prototype.formatToParts ([ value ])</h1>


Making the value argument to Intl.NumberFormat.prototype.formatToParts optional is consistent with the DateTimeFormat version of the function. But it doesn't seem to me that it should be.

There's a somewhat-understandable default value that can be applied for dates, namely the current date/time. But there's not really a good default value for numbers. You could argue for it being zero, but wouldn't it be more sensible for the user to pass 0 in that case? Or you could do what the current patch seems to do and make the default NaN (probably not by deliberate choice? it's just a consequence of ToNumber(undefined) evaluating to it). But it hardly seems like people will generally want to format NaN into parts. As often as not, NaN appearing somewhere means some earlier step in execution went awry, and now things are in la-la land.

I think you should make the value argument here non-optional by removing the brackets and eliminating the step where "value is not provided". Just let lack-of-argument be filled in with undefined by normal spec semantics. This won't stop users from failing to provide the argument, and if they don't pass the argument, they'll still get dumb NaN-handling. But it'll make Intl.NumberFormat().formatToParts.length have a value consistent with how people are expected to use it, and will overwhelmingly use it.

hey @jswalden, IIRC, the decision was based on the idea to align with format: Intl.NumberFormat.prototype.format([value]), which is specified here: https://tc39.github.io/ecma402/#sec-number-format-functions, and specifically, the idea of the progression from format to formatToParts has a way to evolve your ui. If moving from format to formatToParts requires extra logic, then it seems to me like a refactor hazard.

@caridy This wouldn't change any logic -- it would simply change the .length of the function, which wouldn't be consulted by anything using .format directly (because if you know the identity of your callee that way, you wouldn't have any reason to check its .length). If you wanted to be more clear that there's no semantic change in the function's behavior, and that the only change is to the value of its .length, you could have the exact same effect by adding, "The value of the length property of the formatToParts method is 1." just as Intl.Collator.supportedLocalesOf has such language.

/cc @zbraniecki @littledan

I think I agree with @jswalden here. The current default behavior doesn't make much sense (NaN), so I'm ok with requiring an argument.

@zbraniecki I don't think that's what @jswalden is proposing. He is just proposing to set the length to 1, but keeping value as optional.

It would be nice for value to be truly non-optional, but the ship's sailed on having JS APIs that enforce arity. So I think only that the length should be 1, not 0. Intl.NumberFormat().formatToParts() would act as if NaN were passed, even though it's dumb and doesn't make much sense, because it's how JS APIs generally work, that failure to provide an argument is identical to passing undefined for that argument.

I'm supportive of length to be 1, and we can probably have a discussion about whether or not we want value to be optional, it is not too late for that!

I guess I wanted @jswalden's opinion to be the one I presented ;) #argumentumadverecundiam

* NumberFormat.prototype.formatToParts() * DateTimeFormat.prototype.formatToParts()

caridy · 2016-08-14T19:08:02Z

merged as 4a711f0

`Intl.NumberFormat.formatToParts` was first propsed in tc39#30. The spec for it was created in tc39#79 and merged in tc39#100. Due to browser implemantations not being ready at the time, it was moved back to Stage 3 in tc39#101. The internal refactorings were kept in master and the user-facing method `formatToParts` was removed from the spec in tc39#102. As of August 1st, 2017, `Intl.NumberFormat.prototype.formatToParts` has shipped in two engines (behind a flag): SpiderMonkey and V8. This PR brings `Intl.NumberFormat.formatToParts` back as Stage 4 proposal. > const usd = Intl.NumberFormat('en', { style: 'currency', currency: 'USD' }); > usd.format(123456.789) '$123,456.79' > usd.formatToParts(123456.789) [ { type: 'currency', value: '$' }, { type: 'integer', value: '123' }, { type: 'group', value: ',' }, { type: 'integer', value: '456' }, { type: 'decimal', value: '.' }, { type: 'fraction', value: '79' } ] > const pc = Intl.NumberFormat('en', { style: 'percent', minimumFractionDigits: 2 }) > pc.format(-0.123456) '-12.35%' > pc.formatToParts(-0.123456) [ { type: 'minusSign', value: '-' }, { type: 'integer', value: '12' }, { type: 'decimal', value: '.' }, { type: 'fraction', value: '35' }, { type: 'literal', value: '%' } ]

`Intl.NumberFormat.formatToParts` was first propsed in tc39#30. The spec for it was created in tc39#79 and merged in tc39#100 (with follow-ups). Due to browser implementations not being ready at the time, it was moved back to Stage 3 in tc39#101. The internal refactoring were kept in master and the user-facing method `formatToParts` was removed from the spec in tc39#102. As of August 1st, 2017, `Intl.NumberFormat.prototype.formatToParts` has shipped in two engines (behind a flag): SpiderMonkey and V8. This PR brings `Intl.NumberFormat.formatToParts` back as Stage 4 proposal. > const usd = Intl.NumberFormat('en', { style: 'currency', currency: 'USD' }); > usd.format(123456.789) '$123,456.79' > usd.formatToParts(123456.789) [ { type: 'currency', value: '$' }, { type: 'integer', value: '123' }, { type: 'group', value: ',' }, { type: 'integer', value: '456' }, { type: 'decimal', value: '.' }, { type: 'fraction', value: '79' } ] > const pc = Intl.NumberFormat('en', { style: 'percent', minimumFractionDigits: 2 }) > pc.format(-0.123456) '-12.35%' > pc.formatToParts(-0.123456) [ { type: 'minusSign', value: '-' }, { type: 'integer', value: '12' }, { type: 'decimal', value: '.' }, { type: 'fraction', value: '35' }, { type: 'literal', value: '%' } ]

stasm force-pushed the number-format-to-parts branch from e358e35 to a3beb10 Compare February 19, 2016 10:26

This was referenced Feb 19, 2016

[4th Edition] DateTimeFormat.formatToParts() #64

Closed

Extend Formatters to allow for token formatting #30

Closed

stasm reviewed Feb 19, 2016
View reviewed changes

stasm reviewed Feb 26, 2016
View reviewed changes

stasm mentioned this pull request Feb 26, 2016

NumberFormat.formatToParts() andyearnshaw/Intl.js#149

Merged

stasm mentioned this pull request Feb 29, 2016

DateTimeFormat.formatToParts fixes zbraniecki/ecma402#1

Merged

caridy changed the title ~~NumberFormat.prototype.formatToParts()~~ [4rd Edition] NumberFormat.prototype.formatToParts() Feb 29, 2016

stasm force-pushed the number-format-to-parts branch from e3f5a58 to 8c3ee7e Compare March 1, 2016 10:10

littledan reviewed Mar 18, 2016
View reviewed changes

zbraniecki reviewed May 6, 2016
View reviewed changes

stasm force-pushed the number-format-to-parts branch from be83e3f to a131147 Compare June 10, 2016 13:46

stasm mentioned this pull request Jul 28, 2016

NumberFormat.prototype.formatToParts tests tc39/test262#736

Closed

jswalden reviewed Aug 5, 2016
View reviewed changes

caridy added a commit that referenced this pull request Aug 14, 2016

resolving conflicts for PR #79

c1f61e1

caridy mentioned this pull request Aug 14, 2016

FormatToParts: aggregation of PR #64 and PR #79 #100

Merged

caridy added a commit that referenced this pull request Aug 14, 2016

FormatToParts: aggregation of PR #64 and PR #79 (#100)

4a711f0

* NumberFormat.prototype.formatToParts() * DateTimeFormat.prototype.formatToParts()

caridy closed this Aug 14, 2016

rxaviers changed the title ~~[4rd Edition] NumberFormat.prototype.formatToParts()~~ [4th Edition] NumberFormat.prototype.formatToParts() Aug 15, 2016

caridy added a commit that referenced this pull request Aug 30, 2016

restoring abstract from PR #79

4fabb4b

caridy added a commit that referenced this pull request Aug 30, 2016

restoring abstract from PR #79

63439ff

stasm mentioned this pull request Aug 8, 2017

Re-expose Intl.NumberFormat.formatToParts #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4th Edition] NumberFormat.prototype.formatToParts() #79

[4th Edition] NumberFormat.prototype.formatToParts() #79

stasm commented Feb 19, 2016

caridy commented Feb 19, 2016

stasm commented Feb 19, 2016

stasm Feb 19, 2016

stasm commented Feb 19, 2016

stasm commented Feb 25, 2016

stasm Feb 26, 2016

stasm commented Feb 26, 2016

stasm commented Mar 1, 2016

littledan Mar 18, 2016

zbraniecki Mar 23, 2016

zbraniecki commented Mar 29, 2016

zbraniecki May 6, 2016

caridy May 6, 2016

caridy commented May 12, 2016

stasm commented May 14, 2016

stasm commented May 19, 2016

caridy commented May 19, 2016

caridy commented Jun 9, 2016

stasm commented Jun 10, 2016

caridy commented Jun 10, 2016

stasm commented Jun 10, 2016

jswalden Aug 5, 2016

caridy Aug 6, 2016

jswalden Aug 8, 2016

caridy Aug 14, 2016

zbraniecki Aug 15, 2016

caridy Aug 15, 2016

jswalden Aug 15, 2016

caridy Aug 15, 2016

zbraniecki Aug 16, 2016

caridy commented Aug 14, 2016

[4th Edition] NumberFormat.prototype.formatToParts() #79

[4th Edition] NumberFormat.prototype.formatToParts() #79

Conversation

stasm commented Feb 19, 2016

caridy commented Feb 19, 2016

stasm commented Feb 19, 2016

Choose a reason for hiding this comment

stasm commented Feb 19, 2016

stasm commented Feb 25, 2016

Choose a reason for hiding this comment

stasm commented Feb 26, 2016

stasm commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy commented May 12, 2016

stasm commented May 14, 2016

stasm commented May 19, 2016

caridy commented May 19, 2016

caridy commented Jun 9, 2016

stasm commented Jun 10, 2016

caridy commented Jun 10, 2016

stasm commented Jun 10, 2016

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

caridy commented Aug 14, 2016