search

maciejhirsz / json-rust

JSON implementation in Rust
Apache License 2.0
563 stars 63 forks source link

`Eq` trait implementation on `Number` #183

Open timothee-haudebourg opened 4 years ago

timothee-haudebourg commented 4 years ago

I'm wondering why Eq is not implemented for the Number struct. At first, I thought it was because Number can also represent NaN for which we usually expect that NaN != NaN. However a rapid glance at the implementation showed me that in the implementation of PartialEq, we have NaN == NaN.

Having Eq would be useful to use Number or even JsonValue in containers like HashSet.

Also why do you need to represent NaN in Number? As far as I know there is no NaN in JSON, right?

timothee-haudebourg commented 4 years ago

A Hash trait implementation would also be nice.

PurpleMyst commented 4 years ago

Might be interesting to use noisy_float or something.

In JavaScript, JSON.stringify(NaN) returns null. I think having NaN at all near JSON is just an error waiting to happen.

PurpleMyst commented 4 years ago

If the NaN issue is cleared up I'd love to implement this, it seems pretty simple.

maciejhirsz commented 4 years ago

NaN on Number is here purely for conversions from f64. I'm okay with it being removed (along with From) as long as there is a conversion from f64 for JsonValue that produces Null for NaN floats (since that seems to be the expected behavior).

@PurpleMyst with the stuff mentioned above, if you want to submit a PR, go for it :).