decimal
decimal copied to clipboard
alkis
Introduce feature serde_lossy that can deserialize u64, i64, f64
The goal is to be able to deserialize json like {a: 1, b: 2.5} into {a: d128, b: d128} using serde.
I think something "opt-in" is needed, at least for floating point, because serde will parse "numbers with comma" as f64, possibly losing precision. (see my changes to Readme.md)
I'm not 100% sure about this approach, but this works and is opt-in.
You can test this using cargo test --features serde_lossy.
An alternative approach is to expose an alternative visitor and implement visit_f64() etc. for that visitor. That alternative visitor can then be used through #[serde(deserialize_with = ...)]. However that is much more cumbersome, as you'd need to annotate every field with #[serde(deserialize_with = ...)] and I'm not sure how to handle e.g. BTreeMap<String, d128> with that approach.
Let me know what you think. I'm happy to implement different approaches you might prefer.
I have been super busy the last couple of weeks. It will probably take a week for me to find some time to take a look.
On Wed, May 17, 2017 at 3:19 PM, Kosta [email protected] wrote:
Hi!
Were you able to take a look at this? What do you think?
Cheers, Kosta
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/alkis/decimal/pull/33#issuecomment-302087504, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQO_ejV19Wm2cMYcZWr4G-6-9RUgyTsks5r6vPagaJpZM4NUjvR .
Finally I found some time to take a look. I think this added complexity is not necessary. One can parse numbers in a lossy way by defining two structs: one with int/float and then convert to the struct with d128.
The conversion from integer types to decimal is never lossy, perhaps that we can do anyway? WDYT?
Yeah I think we can always do integer conversion, the only thing I'm not sure about is whether is violates "principle of least surprise": If I want to deserialize to d128 (which is supports "decimal points"), at first glace I find it odd if I can deserialize {"value": 5} but not {"value": 5.5}. Of course if I know about the serde internals and "lossiness" this makes sense - but it's not intuitive. That's why I hid both behind that feature flag.
I'm not 100% sure though, I guess it's your call :)
I find the "two structs" solution very cumbersome. Would you merge instead: a pub deserialize_lossy() function that can be used with #[serde(deserialize_with=...)]?
Can't the user provide a deserialize_with= function on their own? It shouldn't be too hard.
Do you want to add lossless integer conversion?
Just some more thoughts on this. When you work with decimal, it is usually because you know floating point does not work for you. So we can assume the user knows they can't represent many decimals with floating point numbers. If they are aware of the issues I think they will understand why we can deserialize "1.4" but not 1.4 and also why we serialize 1.4 into "1.4" and not 1.4. Perhaps all that is necessary here is an FAQ?
Sorry for the late reply! (I'm on paternal leave atm)
What about the following: Discard this PR and the "lossy" feature.
I can instead implement:
- Allow deserialization of integers
- Allow Provide a function you can pass to serde's
deserialize_with, e.g.derserialize_lossy. (Yes, such a function is easy to write but there is no need for everyone to write their own) - Add a FAQ entry why integers can be deserialized but floating points only more cumbersomely.
Sounds good?