Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Decimal128 to Intl.NumberFormat #104

Merged
merged 23 commits into from
May 17, 2024
Merged

Conversation

jessealama
Copy link
Collaborator

The intention here is to make Decimal128 objects valid inputs to the various Intl.NumberFormat functions. One of the guiding examples is that new Intl.NumberFormat("de-DE").format(new Decimal128("-42.0")) should produce "42,0". AFAICS, it's enough to add support for Decimal128 in the ToIntlMathematicalValue abstract operation, but if I'm overlooking something, please let me know!

Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall 402 parts LGTM, note that you would also have to tweak the NumberFormat functions to accept the Decimals and pass them down to ToIntlMathematicalValue for this to work.

@jessealama
Copy link
Collaborator Author

Overall 402 parts LGTM, note that you would also have to tweak the NumberFormat functions to accept the Decimals and pass them down to ToIntlMathematicalValue for this to work.

AFAICS it's enough to tweak ToIntlMathematicalValue. For instance, Intl.NumberFormat.format ought to work with Decimal128 objects, with this change, since (AFAIU) in the spec the argument to format is passed right away to ToIntlMathematicalValue. The other functions, e.g. formatToParts and formatRange, similarly use that AO right away to convert their argument(s) to mathematical values. But maybe I'm missing something.

@jessealama jessealama marked this pull request as ready for review March 18, 2024 10:21
@jessealama
Copy link
Collaborator Author

@sffc How does this look?

AFAICS the relevant NumberFormat functions all ought to handle Decimal128 objects as arguments, since the spec routes things through the ToIntlMathematicalValue AO. But maybe I'm missing something.

@sffc
Copy link

sffc commented Mar 18, 2024

This function makes Decimal128 work with the Intl.NumberFormat.prototype.format[Range][ToParts] functions, but it doesn't retain the tailing zeros as the OP suggests:

One of the guiding examples is that new Intl.NumberFormat("de-DE").format(new Decimal128("-42.0")) should produce "42,0".

@jessealama
Copy link
Collaborator Author

This function makes Decimal128 work with the Intl.NumberFormat.prototype.format[Range][ToParts] functions, but it doesn't retain the tailing zeros as the OP suggests:

One of the guiding examples is that new Intl.NumberFormat("de-DE").format(new Decimal128("-42.0")) should produce "42,0".

I took another look and found out what I was missing. Indeed, as @ryzokuken also said, it's not enough to extend the definition of Intl mathematical value. I've now tweaked the definitions of the various format functions to accept decimals. AFAICS, we should now be able to handle the initial motivating example. (At least, that's my intent.) Are we getting there?

Copy link

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

jessealama and others added 7 commits March 21, 2024 11:25
Set up the algorithm to work with both mathematical values
and Decimal128 objects by initializing relevant variables to
`undefined` straight away, and set their values depending on
the type of the argument of the abstract operation.
Also, remove `Call`, use of `[[accessor]]`, and so on.
@jessealama jessealama merged commit aa24923 into tc39:main May 17, 2024
@jessealama jessealama deleted the intl-spec-work branch May 17, 2024 11:21
@sffc
Copy link

sffc commented May 17, 2024

I'll want to give this another pass when the proposal goes up for stage 2.7.

@sffc
Copy link

sffc commented Jun 11, 2024

It looks like you made Decimal128 another variant of Intl mathematical value, which I think is the right call, but ToIntlMathematicalValue never returns a value of that type. I also see some bugs in the spec text for ToRawFixed.

@ptomato
Copy link
Collaborator

ptomato commented Jun 11, 2024

Might it be possible to replace Intl mathematical values altogether with Decimal128 values?

@sffc
Copy link

sffc commented Jun 13, 2024

Might it be possible to replace Intl mathematical values altogether with Decimal128 values?

This may have worked before NFv3 but at this point it would probably break the web since Intl.NumberFormat allows higher precision than Decimal128

@jessealama
Copy link
Collaborator Author

Might it be possible to replace Intl mathematical values altogether with Decimal128 values?

This may have worked before NFv3 but at this point it would probably break the web since Intl.NumberFormat allows higher precision than Decimal128

I think the best we can do is to bolt Decimal128 values onto Intl mathematical values (done in #163), handle them as-is (that is, without canonicalization/normalization) as far as possible, and then impute additional data (e.g., trailing zeroes) in case an Intl AO demands more than what Decimal128 can provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants