Fix for number filter checking for numbers #6261
Conversation
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
| @@ -118,7 +118,7 @@ function numberFilter($locale) { | |||
|
|
|||
| var DECIMAL_SEP = '.'; | |||
| function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) { | |||
| if (isNaN(number) || !isFinite(number)) return ''; | |||
| if (!isNumber(number) || !isFinite(number)) return ''; | |||
There was a problem hiding this comment.
This will break numbers which happen to be strings.
Better solution: if ( (number !== number) || !isFinite(number) ), or even just if (!isFinite(number)), because isFinite(NaN) should return false.
There was a problem hiding this comment.
this suppose to fix issue when null converted by numberFilter to 0 instead of empty string. Refer to docs (http://docs.angularjs.org/api/ng.filter:number) this should be If the input is not a number an empty string is returned.
There was a problem hiding this comment.
I'm not convinced, I see no reason why the filter should return an empty string for "123"
There was a problem hiding this comment.
Yes, I agree with you my fix is not fully applicable. It should work for numbers which is strings also. Trying to find more efficient solution
|
Updated solution for #6188. Another problem I found is that currently |
| @@ -118,7 +118,7 @@ function numberFilter($locale) { | |||
|
|
|||
| var DECIMAL_SEP = '.'; | |||
| function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) { | |||
| if (isNaN(number) || !isFinite(number)) return ''; | |||
| if (!isNumber(parseFloat(number))) return ''; | |||
There was a problem hiding this comment.
This is getting even weirder than it was this morning.. This is not right :(
There was a problem hiding this comment.
Yeah, tried to make it work as expected with any possible data types
|
Ok, final version. Now it works with passing null, numbers as strings, and numbers. And returns empty string for null. |
| @@ -118,7 +118,7 @@ function numberFilter($locale) { | |||
|
|
|||
| var DECIMAL_SEP = '.'; | |||
| function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) { | |||
| if (isNaN(number) || !isFinite(number)) return ''; | |||
| if (!isFinite(number) || !number) return ''; | |||
There was a problem hiding this comment.
This will turn 0 into the empty string....
There was a problem hiding this comment.
if (number == null || !isFinite(number)) return '';There was a problem hiding this comment.
Might want to also add || isObject(number) to prevent processing arrays as numbers
|
Fixed also for arrays |
|
Okay, this is relatively good, now all you need is some tests. It might be worth re-ordering those tests at the beginning of the filter for perf reasons, but I'm not terribly concerned with it. |
|
Ok, done :) |
|
Okay, again, we need tests! I want to see a test verifying that the number filter on EG: expect(number(null)).toBe('');
expect(number(undefined)).toBe('');
expect(number({})).toBe('');
expect(number([])).toBe('');
expect(number(+Infinity)).toBe('');
expect(number(-Infinity)).toBe('');under the |
Fix for number filter when passing not a number to a filter. isNaN is broken, so we should use isNumber function in formatNumber function. Close #6188
|
Sorry, did not get the first time |
|
Good stuff, LGTM =) I think we can probably get this in today, lemmec |
|
Nice :) Thank you |
|
Lgtm |
|
Thanks for the work. Unfortunately I just realized this will break non-primitive Number/String objects, but I don't think that's going to be a problem, and if anyone complains about it, it can be sorted out later. |
…ings to the empty string The previous code for filtering out non-finite numbers was broken, as it would convert `null` to `0`, as well as arrays. This change fixes this by converting null/undefined/NaN/Infinity/any object to the empty string. Closes angular#6188 Closes angular#6261
Fix for number filter when passing not a number to a filter. isNaN is
broken, so we should use isNumber function in formatNumber function.
Close #6188