Decode: allow integers to be unmarshaled into floats #841
Conversation
|
Thank you for the pull request! I was actually looking into this the other day. It's something that go-toml v1 used to allow, but I removed in go-toml v2. My reasoning was to avoid the loss of precision, and it's kind of a slippery slope to allow automatic conversions. Can you point out in the specification where floats can be expressed like integers? I'm seeing this:
Anyway, I think I'm coming around to it, given that |
|
For me it's this one:
Specifically, the "if used" seems to imply that this is allowed. On closer inspection, I do now see the "and/or" wording for decimal points and exponents, so this could refer to the case where we have an exponent, so I think I misread the docs, oops. I don't think the comparison with That being said, I think it does make sense to allow this. There is no exact mapping of data types from TOML to go, so when unmarshalling I think the question should really be if the value can be assigned in a sensible way. In the case of integers, it generally is assignable to a float right? The argument that we lose precision also seems inconsistent, since we also lose precision with floats:
|
The thing that worries me is the precision loss for larger integers into floats ( On the one hand, we could implement a bound check for those values and error out, or we could still do it and put it on the user to know what they are doing. One of the guidelines for this project is "no surprises", but I think both behaviors could be surprising. What do you think? |
I definitely agree with "no surprises" ! This does need some discussion. I'll invent some cases for illustration. Surprise from current state
Surprise from accepting all ints even with precision loss
Surprise from accepting ints but only without precision loss
My personal and totally biased conclusion is that allowing ints and accepting precision loss has much less confusing surprises, and stays consistent with the behavior of the standard library. The latter is also the reason I think this is the best toml package out there. EDIT upon writing this I realized that parsing values like 12341234123412341234 would fail since they don't fit into an |
|
I think you're right, this seems like the right tradeoff for this lib. Thank you for the patch and the discussion! |
pelletier
changed the title
pelletier
changed the title
This issue was raised and is described in #835
Float values do not require a floating point according to toml documentation: https://toml.io/en/v1.0.0#float
This PR allows integer toml values to be decoded into float64 and float32 fields.
There is no conversion check but as far as I can tell any int64 value can always be converted to a float32 or float64, albeit with potential loss of precision.
I am not sure I got the style right when it comes to tests - I was looking for test(s) where all possible types of values for a field are unmarshaled but found no such test.
stdout of
./ci.sh coverage -d v2: