Bug: Fix precision for currency to use local specification #5672
Conversation
ghost
assigned 
|
I may not have labeled this well enough initially. This is should be considered a bug in the implementation. All indicators show that decimal precision for locales are supported in angular, and yet this bug removes that feature. |
|
@park9140 , did you have any update on this? I'm interested in seeing this issue fixed. |
|
@Juanmcuello, this pull request is still fine as far as I know, but I haven't seen any response from any angular people. |
|
@petebacondarwin, you seem like someone associated with bugs. Is it possible this could get fixed or at least tagged correctly as a bug? |
| "lgSize": 3, | ||
| "macFrac": 0, | ||
| "maxFrac": 3, | ||
| "minFrac": 0, |
There was a problem hiding this comment.
Does the closure localization stuff actually give us these?
There was a problem hiding this comment.
I don't think we can ask formatNumber() to figure out the locale precision, because it's a static routine and doesn't have access to the locale info from ngLocale, it needs to be determined in the currency filter itself.
There was a problem hiding this comment.
@caitp, yes, closure localization does provide the localization pattern information.
As for the the formatNumber() figuring out the locale precision.
Format number does not do this, the actual currency filter does. The locale provides the "pattern" object by which a number formatter is configured.
The main change here is to the currency filter, which was locking the precision by passing the fractionSize parameter of it's formatNumber call to 2.
The secondary change is that the formatNumber function was not utilizing the passed pattern maxFrac value when determining precision.
These tests do not verify the formatNumber functionality change. Should I add a test for that?
|
@caitp, any idea when we might be able to finish this? We are releasing our project to the public over the next month and this is very important to our international customers. I would rather I didn't have to create a hack for this and just have it in angular. |
|
@park9140 there are things I don't really like about this, particularly moving the locale reading into formatNumber, when that should be the caller's responsibility. I think we can get this in either today or next week if we can change that. @IgorMinar thoughts? |
|
@caitp, Just incase you missed it I replied to your previous comment on the code. Basically the local provides the I don't think there is any way to get away from having a I have another pull request around this |
|
I understand that, however there are plenty of properties to pick from in the locale files, and some are pertinent to local representations of numbers for math, you might have different ones for currency, and so forth. So I don't think it's correct to add that logic to formatNumber(). Why not simply have the caller decide which property to use, and default to 2 if there is no locale object in the first place? |
| @@ -46,7 +46,7 @@ function currencyFilter($locale) { | |||
| var formats = $locale.NUMBER_FORMATS; | |||
| return function(amount, currencySymbol){ | |||
| if (isUndefined(currencySymbol)) currencySymbol = formats.CURRENCY_SYM; | |||
| return formatNumber(amount, formats.PATTERNS[1], formats.GROUP_SEP, formats.DECIMAL_SEP, 2). | |||
There was a problem hiding this comment.
@caitp, this is the primary fix here. The formatNumber function below already supported doing the formatting precision based on the pattern already passed here. So, I removed the obviously accidental force to the 2 decimal precision.
|
Any help I can provide to get this bug resolved? |
|
@caitp, any movement on this yet? I'm going to be replacing the currency filter entirely in our application because this hasn't been pulled. |
| @@ -122,7 +121,7 @@ function formatNumber(number, pattern, groupSep, decimalSep, fractionSize) { | |||
| var hasExponent = false; | |||
| if (numStr.indexOf('e') !== -1) { | |||
| 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) { | |||
There was a problem hiding this comment.
match[2] can just test if it's truthy, since there's only one possible value other than the empty string
There was a problem hiding this comment.
also, if fractionSize isn't defined, and pattern.maxFrac isn't defined, then it's undefined + 1 (NaN) --- you don't want that --- really, treat fractionSize as mandatory and make the number filter set it to whatever it wants to instead. Or else default to 2 instead if neither are specified
There was a problem hiding this comment.
@caitp, the existing code structure here it is appropriate that this become NaN. This forces it to false when a fraction size is not set. Forcing this to a defined default would change the formatting significantly since currently in the undefined state of the pattern.maxFrac and fractionSize it defaults to showing the full precision of the number.
|
There are many ways to fix this. I made a PR on #7582 to solve the same issue. I did accept the Angular Regulations and everything else. But nothing was pushed, yet! |
Currency improperly forces the precision to 2 decimal places for all currency formmating, overriding the locale settings.
Pull request restores the intended functionality to apply the local based precision values.