Breaking changes in number parsing after 1.2.0

[eric-parsons]

Hello,

After upgrading to 1.2.x, the behavior of number parsing has changed significantly, and seems somewhat broken:

Version 1.1.2:

globalize.locale("en");
globalize.parseNumber("75") // 75
globalize.parseNumber("$75.00") // 75
globalize.parseNumber("75%") // 0.75
globalize.parseNumber("75", { style: "percent" }) // 75 ?
globalize.parseNumber("75%", { style: "percent" }) // 0.75

Version 1.2.0:

globalize.parseNumber("75") // 75
globalize.parseNumber("$75.00") // NaN ?
globalize.parseNumber("75%") // NaN ?
globalize.parseNumber("75", { style: "percent" }) // NaN
globalize.parseNumber("75%", { style: "percent" }) // 0.75

So, parsing currency is no longer possible at all. Parsing percent now only works if style: "percent" is specified, in which case it will only recognize percent input. I'm not sure if this was the intended use of this flag all along, but it makes the API significantly less useful. If I want to accept either "0.75" or "75%" as valid input to mean 0.75, then I must do my own pre-flight check to see whether or not it's a percent. Forcing the calling code to make this type of check makes it likely that it will fail for some combination of input and locale, and defeats the purpose of using an API like Globalize in the first place. The old behavior of this flag didn't seem quite right either though, since it seemingly had no effect at all.

[rxaviers]

Hello @eric-brechemier, thanks for bringing these points to our attention. On version <1.2.0, the number parser had significant bugs as you demonstrated above, also see #292 and #353. All these have been fixed in 1.2.x.

The parser now works correctly. All the 1.2.0 examples you demonstrate seems correct to me.

Percentages

Assume .parseNumber() accepted either "0.75" or "75%" as valid inputs to mean 0.75 as you mention. Considering you want both, that would be great, but now consider you want to strictly accept one or the other, not being able to control what .parseNumber() accepts would be a pain.

The implemented .parseNumber() correctly parses what you tell it to. Think of it as a building block for your own logic. If you want to accept either "0.75" or "75%" as valid input, simply do it:

function parseNumerberOrPercentage(numberOrPercentageString) {
  return globalize.parseNumber(numberOrPercentageString) ||
    globalize.parseNumber(numberOrPercentageString, {style: "percent"});
}

Note that on date parser, we chose to go with the same specific style/patterns instead of trying to guess several patterns for the develop.

Forcing the calling code to make this type of check makes it likely that it will fail for some combination of input and locale

I can't think of any, if you find please just let us know.

and defeats the purpose of using an API like Globalize in the first place

Not true, the real value of Globalize is to deal with the i18n tough problems, not to avoid you do the helper above (that is too obvious). The real problem parsing a number goes in places like parsing a different numbering systems (like arabic), loose matchings like #292, or even making sure you don't parse a wrong currency or a wrong percentage value.

globalize.parseNumber("$75.00")

This should indeed return a NaN. You don't want to parse €2 as 2 and assume this is USD for example. For currency parsing, a proper currency parser should be used instead, i.e., currencyParser().

[rxaviers]

I closed this issue, because I don't think this is a bug. I still think it was very important you raised these points, because it shows we might do a better job explaining them.

Do you have a suggestion how we could improve that?

In our documentation, we have https://github.com/globalizejs/globalize/blob/master/doc/api/number/number-parser.md.

Thanks again

[eric-parsons]

Hi Rafael,

OK, fair enough -- if that's how it's supposed to work then I can accept that. I hadn't thought to parse the same input twice both with and without the percent flag. Good idea! That gets around the need to perform any pre-parsing by the calling code, so you can disregard my comments regarding that. Though careful -- the example you gave fails for input "0" ;-). Nevertheless the general idea would work.

As far as improving the documentation, my only suggestion would be to make it a bit clearer that the style: "percent" flag is required in order to recognize percent input. Intuitively, I would expect that style: "percent" would only recognize percent (which it does), but that if I wanted decimal only then I would have to specify something similar like style: "decimal" (for symmetry with the formatting API), and that specifying no style at all would accept either. But, that's not how it works.

Thanks for you time and quick response.

[rxaviers]

Hi, thanks for your understanding.

Though careful -- the example you gave fails for input "0" ;-). Nevertheless the general idea would work.

0 will fall into the decimal style, but not for the percentage style. Right? I don't see where it would fail.

expect that style: "percent" would only recognize percent (which it does), but that if I wanted decimal only then I would have to specify something similar like style: "decimal" (for symmetry with the formatting API)

That's what we have considering that style: "decimal" is the default 😄 https://github.com/globalizejs/globalize/blob/master/doc/api/number/number-parser.md#parameters

As far as improving the documentation

It would be awesome if you could send a PR with your suggested improvement. :)

Thanks

[eric-parsons]

0 will fall into the decimal style, but not for the percentage style. Right? I don't see where it would fail.

In the following code:

function parseNumerberOrPercentage(numberOrPercentageString) {
  return globalize.parseNumber(numberOrPercentageString) ||
    globalize.parseNumber(numberOrPercentageString, {style: "percent"});
}

if numberOrPercentageString is the string "0", then the output of globalize.parseNumber(numberOrPercentageString) will be 0, which is "falsy". As a result, the || operator will evaluate the RHS expression instead, which evaluates to NaN. So parseNumberOrPercentage("0") will return NaN, whereas it should return 0. Anyway, no big deal, I just wanted to point it out in case someone copies and pastes that code.

That's what we have considering that style: "decimal" is the default 😄

You're right, don't know how I missed that.

[rxaviers]

will be 0, which is "falsy".

True 🙈, thanks 😄