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

Throw roundingIncrement if maximumFractionDigits is not 0 #81

Closed
FrankYFTang opened this issue Dec 3, 2021 · 10 comments · Fixed by #85
Closed

Throw roundingIncrement if maximumFractionDigits is not 0 #81

FrankYFTang opened this issue Dec 3, 2021 · 10 comments · Fixed by #85
Labels
has-consensus Has consensus and ready to implement

Comments

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Dec 3, 2021

We currently read the roundingIncrement the following
https://tc39.es/proposal-intl-numberformat-v3/out/numberformat/proposed.html

21. Let roundingIncrement be ? GetNumberOption(options, "roundingIncrement,", 1, 5000, 1).
22. If roundingIncrement is not in « 1, 2, 5, 10, 20, 25, 50, 100, 200, 250, 500, 1000, 2000, 2500, 5000 », throw a RangeError exception.

But {roundingIncrement:5, maximumFractionDigits:0 } is the same as
{roundingIncrement:50, maximumFractionDigits:1 } and
{roundingIncrement:500, maximumFractionDigits:2 } and
{roundingIncrement:5000, maximumFractionDigits:3 }

How about we ALSO ADD the following AFTER the above lines

22. If maximumFractionDigits is not 0 and roundingIncrement is not in « 1, 2, 5, 25 », throw a RangeError exception.

This will ensure these values of roundingIncrement will not be used while not needed.

so while maximumFractionDigits > 0, roundingIncrement can only be 1, 2, 5, 25 . In this way, we disallow different combination of roundingIncrement+maximumFractionDigits produce the same result.
@sffc

@FrankYFTang
Copy link
Contributor Author

@sffc
Copy link
Collaborator

sffc commented Dec 6, 2021

I agree that this is an area we could change, but I think I would prefer adjusting the options after reading them, rather than throwing an exception. The reason is that there is actually a use case for this: { roundingIncrement:50, minimumFractionDigits:2, maximumFractionDigits:2 } (with minimumFractionDigits) results in "0.00", "0.50", "1.00", "1.50", ..., which is a behavior that cannot be gotten if the options bag is flattened up one level.

@sffc
Copy link
Collaborator

sffc commented Dec 7, 2021

The behavior I'd propose to fix this and other issues is this: If roundingIncrement is used, require that minimumFractionDigits == maximumFractionDigits.

This is a requirement that could be relaxed in the future once ICU supports it in the skeleton (https://unicode-org.atlassian.net/browse/ICU-21866). I believe this new requirement fixes Frank's issue in the OP, and it is also a workaround to make this proposal implementable with ICU 70.

@sffc sffc added the discuss Needs discussion to make progress label Dec 7, 2021
@FrankYFTang
Copy link
Contributor Author

The behavior I'd propose to fix this and other issues is this: If roundingIncrement is used, require that minimumFractionDigits == maximumFractionDigits.

I do not mind you ALSO propose that, but how would that change the fact that

{roundingIncrement:5, maximumFractionDigits:0 } is the same as
{roundingIncrement:50, maximumFractionDigits:1 } and
{roundingIncrement:500, maximumFractionDigits:2 } and
{roundingIncrement:5000, maximumFractionDigits:3 }

??? since minimumFractionDigits is neither in these options. I do not think your proposal can solve this issue. It could solve the other issue only.

@sffc
Copy link
Collaborator

sffc commented Dec 7, 2021

With my proposed requirement, all of those options bags except the first one would be illegal, because minimumFractionDigits defaults to 0, so in cases 2-4, minimumFractionDigits != maximumFractionDigits.

@FrankYFTang
Copy link
Contributor Author

you missed my point, then how about
{roundingIncrement:5, minimumFractionDigits:0, maximumFractionDigits:0 } is the same as
{roundingIncrement:50, minimumFractionDigits:1, maximumFractionDigits:1 } and
{roundingIncrement:500, minimumFractionDigits:2, maximumFractionDigits:2 } and
{roundingIncrement:5000, minimumFractionDigits:3, maximumFractionDigits:3 }

@sffc
Copy link
Collaborator

sffc commented Dec 7, 2021

you missed my point, then how about {roundingIncrement:5, minimumFractionDigits:0, maximumFractionDigits:0 } is the same as {roundingIncrement:50, minimumFractionDigits:1, maximumFractionDigits:1 } and {roundingIncrement:500, minimumFractionDigits:2, maximumFractionDigits:2 } and {roundingIncrement:5000, minimumFractionDigits:3, maximumFractionDigits:3 }

Those are different, because they result in different numbers of trailing zeros. "5" != "5.0" != "5.00" != "5.000"

@FrankYFTang
Copy link
Contributor Author

ok, I missed that.

@sffc
Copy link
Collaborator

sffc commented Dec 7, 2021

If we want to avoid the exception and perhaps be more forward-compatible, I'm also happy to just set minimumFractionDigits to maximumFractionDigits when roundingIncrement is used.

@sffc
Copy link
Collaborator

sffc commented Jan 13, 2022

2021-01-13 discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-01-13.md#throw-roundingincrement-if-maximumfractiondigits-is-not-0-81

Conclusion: Do as I proposed above and require minFrac=maxFrac when roundingIncrement is being used.

@sffc sffc added has-consensus Has consensus and ready to implement and removed discuss Needs discussion to make progress labels Jan 13, 2022
romulocintra added a commit to romulocintra/proposal-intl-numberformat-v3 that referenced this issue Feb 4, 2022
romulocintra added a commit to romulocintra/proposal-intl-numberformat-v3 that referenced this issue Feb 21, 2022
romulocintra added a commit to romulocintra/proposal-intl-numberformat-v3 that referenced this issue Feb 21, 2022
romulocintra added a commit that referenced this issue Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-consensus Has consensus and ready to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants