Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parseFloat too permissive, e.g. 4,0 taken as 40 #46

Closed
yucca42 opened this issue Jul 6, 2011 · 6 comments · Fixed by #656
Closed

parseFloat too permissive, e.g. 4,0 taken as 40 #46

yucca42 opened this issue Jul 6, 2011 · 6 comments · Fixed by #656
Labels

Comments

@yucca42
Copy link

yucca42 commented Jul 6, 2011

Globalize.parseFloat("4,0",10,"en") yields 40, which is unacceptable. It should either work the same way as the standard parseFloat, parsing a number at the start of its first parameter (yielding 4) or parse the entire attribute as a number, recognizing that it does not fit into the number patterns of the locale (yielding NaN). The latter is preferable, as it would help in detecting data errors (like input "4,0" when a number in the English locale is expected).

It is highly probable that input like 4,0 in English locale is data error. It is extremely improbable that the user really meant the number 40

Technically, parseFloat, upon encountering a group separator, should test that there is a correct number of digits after it, as defined by numberFormat.groupSizes.

Similar considerations apply to parseInt, of course.

@worldspawn
Copy link

This sounds similar to the issue i am having where running as "fr" parseFloat will parse 10,5 as 10.5 but it also parses 10.5 as 10.50. It should return NaN. Its a real pain cause in my case I am submitting these values back to an asp.net app running in French which balks at 10.5 when it wants 10,5... otherwise I probably wouldn't care :)

Unfortunately any value that matches the en-US format will pass, it should really create an reg expression or something to evaluate if the entire string is valid.

@yucca42
Copy link
Author

yucca42 commented Aug 3, 2011

The issue that en-US format is accepted on input is distinct from the problem that a grouping (thousands) separator is accepted all too liberally. It’s a difficult topic, since it might often be useful to allow both 10,5 and 10.5, though things get difficult if the period is used as a thousands separator - you can’t really accept 10.500 as the same as 10,500 if the locale definitions imply that it means 15000.

The French-locale manifestation of the problem I described is that
Globalize.parseFloat('4 0')
yields 40.

@janjonas
Copy link

janjonas commented Oct 3, 2011

I think this problem is related:
The parseFloat() methods allow patterns with multiple thousands group markers like:
Globalize.parseFloat(",,9,,,,,,,9,,,,.99", 10, "en-US") -> is parsed to 99.99
I would expect that parseFloat() returns NaN.

@DavidDeSloovere
Copy link

Maybe adding a 'strictParsing' property might allow both scenarios, without breaking existing code by having the default set to false.
The property could be added to Globalize itself, might be useful for dates or such (can't think of any atm).

@jzaefferer
Copy link
Contributor

This issue still exists in the CLDR rewrite.

@jzaefferer jzaefferer added the bug label Dec 11, 2014
@jzaefferer jzaefferer added this to the 1.0.0 milestone Dec 11, 2014
@rxaviers rxaviers modified the milestones: 1.1.0, 1.0.0 Feb 12, 2015
@rxaviers rxaviers removed this from the 1.1.0 milestone Aug 31, 2015
@rxaviers rxaviers mentioned this issue Aug 31, 2015
6 tasks
@rxaviers
Copy link
Member

This issue should be fixed by #353. I've added notes there to ensure this is tested. I'm closing this issue in favor of the latter to focus efforts. If this is a problem for you, your help is very much appreciated.

ashensis pushed a commit to ashensis/globalize that referenced this issue Mar 17, 2016
Make sure Globalize.locale is a symbol (like enforced by I18n.locale)
rxaviers added a commit that referenced this issue Nov 28, 2016
- Correctly handles prefix and suffix literals; #353;
- Loose Matching: This implementation is now much closer to UTS#35 7.1.2 Loose
  Matching http://unicode.org/reports/tr35/#Loose_Matching and fixes all
  reported cases that related to it, including #288;
- Regression: Drop scientific notation parsing support, which wasn't documented
  anyway and shall be implemented by #533.

Ref #292
Fixes #353

Fixes #46
Fixes #288
Fixes #443
Fixes #457
Fixes #492
Fixes #587
Fixes #644
rxaviers added a commit that referenced this issue Nov 28, 2016
- Correctly handles prefix and suffix literals; #353;
- Loose Matching: This implementation is now much closer to UTS#35 7.1.2 Loose
  Matching http://unicode.org/reports/tr35/#Loose_Matching and fixes all
  reported cases that are related to it, including #288;
- Regression: Drop scientific notation parsing support, which wasn't documented
  anyway and shall be implemented by #533.

Ref #292
Fixes #353

Fixes #46
Fixes #288
Fixes #443
Fixes #457
Fixes #492
Fixes #587
Fixes #644
rxaviers added a commit that referenced this issue Dec 13, 2016
- Correctly handles prefix and suffix literals; #353;
- Loose Matching: This implementation is now much closer to UTS#35 7.1.2 Loose
  Matching http://unicode.org/reports/tr35/#Loose_Matching and fixes all
  reported cases that are related to it, including #288;
- Regression: Drop scientific notation parsing support, which wasn't documented
  anyway and shall be implemented by #533.

Ref #292
Fixes #353

Fixes #46
Fixes #288
Fixes #443
Fixes #457
Fixes #492
Fixes #587
Fixes #644
rxaviers added a commit that referenced this issue Dec 13, 2016
- Correctly handles prefix and suffix literals; #353;
- Loose Matching: This implementation is now much closer to UTS#35 7.1.2 Loose
  Matching http://unicode.org/reports/tr35/#Loose_Matching and fixes all
  reported cases that are related to it, including #288;
- Regression: Drop scientific notation parsing support, which wasn't documented
  anyway and shall be implemented by #533.

Ref #292
Fixes #353

Fixes #46
Fixes #288
Fixes #443
Fixes #457
Fixes #492
Fixes #587
Fixes #644
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants