Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/ng/filter/filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,13 @@ function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) {

var hasExponent = false;
if (numStr.indexOf('e') !== -1) {
hasExponent = true;
var match = numStr.match(/([\d\.]+)e(-?)(\d+)/);
if (match && match[2] == '-' && match[3] > fractionSize + 1) {
if (match && match[2] == '-' && match[3] > (fractionSize || pattern.maxFrac) + 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

numStr = '0';
number = 0;
} else {
formatedText = numStr;
hasExponent = true;
}
}

Expand All @@ -171,10 +171,6 @@ function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) {
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/round
number = +(Math.round(+(number.toString() + 'e' + fractionSize)).toString() + 'e' + -fractionSize);

if (number === 0) {
isNegative = false;
}

var fraction = ('' + number).split(DECIMAL_SEP);
var whole = fraction[0];
fraction = fraction[1] || '';
Expand Down Expand Up @@ -207,12 +203,16 @@ function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) {

if (fractionSize && fractionSize !== "0") formatedText += decimalSep + fraction.substr(0, fractionSize);
} else {

if (fractionSize > 0 && number > -1 && number < 1) {
fractionSize = isUndefined(fractionSize) ? pattern.maxFrac : fractionSize;
if (number > -1 && number < 1) {
formatedText = number.toFixed(fractionSize);
}
}

if (number === 0) {
isNegative = false;
}

parts.push(isNegative ? pattern.negPre : pattern.posPre,
formatedText,
isNegative ? pattern.negSuf : pattern.posSuf);
Expand Down
8 changes: 8 additions & 0 deletions test/ng/filter/filtersSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,14 @@ describe('filters', function() {
expect(number(-1e-6, 6)).toEqual('-0.000001');
expect(number(-1e-7, 6)).toEqual('-0.000000');
});

it('should filter exponentially small numbers when no fraction specified', function() {
expect(number(1e-10)).toEqual('0.000');
expect(number(0.0000000001)).toEqual('0.000');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


expect(number(-1e-10)).toEqual('0.000');
expect(number(-0.0000000001)).toEqual('0.000');
});
});

describe('json', function() {
Expand Down