adobe / json-formula

Query language for JSON documents
http://opensource.adobe.com/json-formula/
Apache License 2.0
19 stars 8 forks source link

Issues with `toNumber` #147

Closed Eswcvlad closed 4 months ago

Eswcvlad commented 5 months ago
  1. toNumber("")
  2. toNumber("", 16)
  3. toNumber("uasdicyb", 16)

Per spec I would expect these cases to return null, but they return 0 instead. At the same time toNumber("uasdicyb") behaves as expected.


  1. toNumber("11", `null`)
  2. toNumber("11", `false`)
  3. toNumber("11", `true`)
  4. toNumber("11", 5.1)
  5. toNumber("11", 4)
  6. toNumber("11", "dwvetrwg")

With how base is defined (i.e. only 2, 8, 10, 16), I kind of expected it to throw some error, especially as it will be, most likely, a literal in practice. Instead it just defaults to 10 with a log message, which seems a bit odd.


toNumber("99XX", 16) returns 99, while at the same time toNumber("99XX") returns null. The latter case seems more correct. At least it would be better for them to behave the same.


toNumber(`false`) and toNumber(`true`) return 0 and 1 respectfully, which looks correct.

At the same time toNumber(`false`, 16) and toNumber(`true`, 16) return 250 and 0, with the latter having a debug log message (i.e. they are converted to string and parsed back). Personally, I would have expected them to return 0 and 1 regardless of base, as a direct conversion to number.


This one is a bit strange... When the argument to toNumber is an actual number, I wasn't quite sure, on what the behavior for base != 10 should be like, but it seemed like it would make most sense to just leave the number as-is. It is still a number and us writing it in base-10 is not that relevant.

It looks like the implementation in such cases casts the number to a base-10 string and parses it back (i.e toNumber(11, 16) returns 17). While I somewhat see the point, when you have a literal as an argument, it looks pretty strange, when it is some variable or expression results.

I doubt this is an important case, but if current behavior is to remain, it should 100% be documented in spec, imo.


The last one is a bit pedantic and is not that important, but still:

  1. toNumber(`[1]`)
  2. toNumber(`[1]`, 16)
  3. toNumber(`{}`)
  4. toNumber(`{}`, 16)

In the implementation these cases throw a TypeError. Either this or null seems logical.

At the same time from the spec point of view, arrays and objects cannot be coerced to string, number or null, but they can be coerced to bool. So they would be coerced to true or false, which is a valid case.

Maybe change the argument type to any and return null to remove this odd corner case? Kind of like it is for toString now.

JohnBrinkman commented 5 months ago

Per spec I would expect these cases to return null, but they return 0

agreed

With how base is defined (i.e. only 2, 8, 10, 16), I kind of expected it to throw some error

toNumber("99XX", 16) returns 99, while at the same time toNumber("99XX") returns null

Agreed. For all the non-base 10 conversions we need to check for valid digits

At the same time toNumber(false, 16) and toNumber(true, 16) return 250 and 0, When the argument to toNumber is an actual number, I wasn't quite sure, on what the behavior for base != 10 should be like

These two cases are the same. In both instances when base is provided we implicitly convert the input argument to a string. That's why the conversion picked up the f from false to return 16 I will change the documentation to state that the base parameter is used only when the input type is string. And have the implementation behave accordingly.

In the implementation these cases throw a TypeError. Either this or null seems logical. At the same time from the spec point of view, arrays and objects cannot be coerced to string, number or null, but they can be coerced to bool. So they would be coerced to true or false, which is a valid case.

I think this gets fixed when we resolve: https://github.com/adobe/json-formula/issues/146 i.e. when the input parameter allows multiple types, then we do not perform any type conversion.