json-rust icon indicating copy to clipboard operation
json-rust copied to clipboard

`Eq` trait implementation on `Number`

Open timothee-haudebourg opened this issue 5 years ago • 4 comments

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 avatar Mar 30 '20 17:03 timothee-haudebourg

A Hash trait implementation would also be nice.

timothee-haudebourg avatar Mar 30 '20 17:03 timothee-haudebourg

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 avatar May 06 '20 15:05 PurpleMyst

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

PurpleMyst avatar May 06 '20 15:05 PurpleMyst

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 :).

maciejhirsz avatar May 08 '20 07:05 maciejhirsz