fix(numberFilter): properly format small number expressed with e notation

[pkozlowski-opensource]

Fixes #10246

[googlebot]

CLAs look good, thanks!

[bobrik]

👍

[lgalfaso]

I think this is not the right check, if fractionSize === 0, then we do not want pattern.maxFrac

[pkozlowski-opensource]

@lgalfaso can we ever go into this case? It could only happen for something like 1e-2 which would be converted to 0.01 anyway. So I think we are good here, unless I'm missing other bcases of course.

[pkozlowski-opensource]

Btw, there is a test that uses fractionSize 0 but then again, if you can think of a test that would fail with the current impl, then let's change things.

[lgalfaso]

of course fractionSize can be 0, it is an optional parameter, but that does not mean that 0 is not a valid value

[lgalfaso]

Why not just add

if (isUndefined(fractionSize)) {
  fractionSize = formats.PATTERNS[0].maxFrac;
}

to the function returned by numberFilter

?

[pkozlowski-opensource]

@lgalfaso hmm, i don't think we can do this as there is a logic to calculate fraction size dynamically.

[lgalfaso]

the function formatNumber is only called from currencyFilter and numberFilter. Are you implying that without this PR then currencyFilter has a bug?

[pkozlowski-opensource]

@lgalfaso no, I'm not suggesting that currencyFilter filter is having the same problem. What I'm saying is that numberFilter has special treatment for the case where the fractionSize wasn't provided - in such a case it will try to "guess" fractionSize from a number itself. To be more concrete let's take this expression {{1.52 | number }} in a locale where max fractionSize equals to 3. In this case we want to output 1.52 and not 1.520 - at least this is the current logic and we've got tests for it - so I'm assuming it is deliberate.

The logic I'm mentioning seats here.

In short: we can't simply default to the pattern's max fraction size in here as it would break the existing logic / tests.

Does it make sense? Or were you having a different idea?

[lgalfaso]

I think where you are going, but I think it would be simpler to add a removeTrailingZeros parameter to numberFilter, this would be always false for currencyFilter and in the case of numberFilter only if fractionSize is undefined. One way or another, formatNumber always get fractionSize

[pkozlowski-opensource]

@lgalfaso not sure I follow the idea of removeTrailingZeros to the numberFilter - this would require breaking change, no? Or did you mean formatNumber ?

Are you positive that code with your idea would be shorter / simpler?

[lgalfaso]

sorry, I meant formatNumber

[pkozlowski-opensource]

OK, then again, we would need to have an alternative PR to compare as I'm not sure that introducing removeTrailingZeros will code shorter / conceptually simpler...

Maybe let's focus on fixing the bug with all its corner cases and then we can discuss a refactoring PR?

[lgalfaso]

@pkozlowski-opensource the formatNumber was refactored many times as number formatting in javascript can be a pain in the neck. On top of that, we are already adding the trailing zeros at https://github.com/angular/angular.js/blob/master/src/ng/filter/filters.js#L204

[lgalfaso]

if there is no fractional size, then we should have the minimum number of trailing zeros, this is it should equal 0

[pkozlowski-opensource]

@lgalfaso this is not how it works for numbers like 0.0001 today. What you are going to get is 0.000 not 0. IMO we should be consistent and format a given number the same way, regardless of the notation used (e or dot). This is why I've explicitly added a test for this. I mean, this test expect(number(0.0000000001)).toEqual('0.000'); is passing without my code change.

[pkozlowski-opensource]

See: http://plnkr.co/edit/3RQ4Vc3EutslbfPgHvJA?p=preview

[pkozlowski-opensource]

@lgalfaso I understand that it is refactored many times and that there are hard cases (like formatting big numbers). But let's not throw everything to one basket - I believe that this is a simple bug with a simple fix and I would like to focus on this here.

[lgalfaso]

Sorry, but I do not feel comfortable with

Maybe let's focus on fixing the bug with all its corner cases and then we can discuss a refactoring PR?

the code is already quite complex, so I want to not add any more complexity

BTW, if we change the behavior for small numbers, it also means that we have to change it for large. E.g 1+e20 | number still outputs the number using scientific notation

[pkozlowski-opensource]

@lgalfaso OK, after spending some more time on this one I agree that the current implementation is fundamentally flawed and shouldn't be used as a base to build on top. The number of corner (and-not-so-corner) cases I've discovered is ... well, pretty scary for me. Most of those are around formatting of e-numbers so people are probably not hitting it that often, but still....

I believe that the tests from this PR are good and I can think of several other tests that would be breaking with the current implementation (BTW: there are even 2 tests in the current master that are contradictory yet still both pass because of the broken impl :-/).

I'm going to close this PR and if my motivation is not dying out I will try to open another PR with more tests so we feel safe when refactoring.

[pkozlowski-opensource]

Thnx for bearing with me Lucas.