Repository navigation
DecimalPipe incorrectly rounds negative numbers when the fraction is .5 #24038
Description
I'm submitting a...
[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question
[ ] Other... Please describe:
Current behavior
{{-2.5 | number:'1.0-0'}} displays -3 in the template
Expected behavior
{{-2.5 | number:'1.0-0'}} should follow the same rules as Math.round(-2.5) and display -2 in the template
Minimal reproduction of the problem with instructions
https://stackblitz.com/edit/angular-gitter-msxufr
Further details
The issue stems from formatNumber, which calls formatNumberToLocaleString, which calls parseNumber, which does Math.abs at https://github.com/angular/angular/blob/master/packages/common/src/i18n/format_number.ts#L316
Afterwards, formatNumberToLocaleString at the end at https://github.com/angular/angular/blob/master/packages/common/src/i18n/format_number.ts#L116 re-adds the negative symbol.
What is the motivation / use case for changing the behavior?
Environment
Angular version: X.Y.Z
Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
For Tooling issues:
- Node version: XX
- Platform:
Others:
Reacted by Максим Иванов, Sergej Grilborzer and Shahmir Noorani
Reacted by dawidgarus
Activity
vicb commentedon May 22, 2018
I agree it's a bit awkward but which behavior is correct is not simple, see the different types of rounding at https://en.wikibooks.org/wiki/Arithmetic/Rounding.
While behaving the same as JS would probably be better, it would also be a breaking change for people relying on the current behavior.
May be one way to fix would be to:
AWAY_FROM_ZERO, i.e. the current behavior.What do you guys think ?
shahmirn commentedon May 22, 2018
@vicb That sounds like a good solution to ensure that it's a non-breaking change and would work for me.
Nosfistis commentedon May 24, 2018
It would be nice to mention the rounding in the docs, and perhaps provide some extra options (flooring, ceiling etc). Currently there is no mention that the number is being rounded.
vicb commentedon May 24, 2018
Would somebody here be willing to submit PRs to
Nosfistis commentedon May 25, 2018
I can prepare a PR if we could agree on the notation. I checked the docs and noticed that
formatNumberToLocaleString()is used in percent and currency pipes as well. It is possible to extend the behaviour to those pipes as well, or keep the same default behaviour.vicb commentedon May 25, 2018
I probably makes sense to extend that to all pipes.
May be you can do a short proposal so that we agree before jumping to the impl.
Also could someone take care of the API docs (describing the current behavior).
Thanks !
s-gbz commentedon May 25, 2018
I will take care of it Monday or Tuesday.
Nosfistis commentedon Jun 11, 2018
I found some time to look into this again, and it seems that no breaking change would occur if we used the same changes on the rest pipes. I have 2 possible solutions for this proposal:
{minIntegerDigits}.{minFractionDigits}-{maxFractionDigits}*{rounding}. Using the*symbol as delimiter seems correct to me in order to differentiate from the previous-and is single character notation, although brackets might resemble more like rounding ({minIntegerDigits}.{minFractionDigits}-{maxFractionDigits}[{rounding}]).In both cases the proposed possible parameter values would comply to the aforementioned wiki and would be:
ceilingflooraway_from_zeroto_zeroto_nearest: The current behaviorvicb commentedon Jul 10, 2018
Docs added in #24303
shahmirn commentedon Jul 10, 2018
@vicb Was the actual underlying bug fixed along with the docs there we added in #24303? I don't see any mention of that in this issue.
This issue shouldn't be closed until that work is done.
angular-automatic-lock-bot commentedon Sep 13, 2019
This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.
Read more about our automatic conversation locking policy.
This action has been performed automatically by a bot.