search

arcnmx / serde-value

Serialization value trees
http://arcnmx.github.io/serde-value/serde_value/
MIT License
43 stars 30 forks source link

Added support for feature = preserve_order #21

Open mitsuhiko opened 5 years ago

mitsuhiko commented 5 years ago

This adds support for the preserve_order feature that is also provided by serde_json. The behavior is similar and comes with the same general downsides that it's a global feature.

arcnmx commented 5 years ago

Thanks for the PR! I'm open to including this, but do have a few points that need addressing:

  1. So we probably also need to add a custom PartialEq to match Hash and Ord here, otherwise it will use indexmap's implementation, which apparently does not preserve ordering. (The next point invalidates the need for this though.)
  2. As this is a global feature, enabling the feature would break comparisons that otherwise expect eq/ord/hash to be ordering agnostic as they are now... We could implement eq/hash easily enough, though would Ord be a pain? I imagine the impl would need to sort the keys first or something.
  3. This changes the public API, and makes multiple breaking changes to it just by enabling the feature. I'm pretty hesitant to do that... We may need to take serde_json's approach of adding a ::map module with a newtype that can be used regardless of the MapImpl chosen.
    • Alternatively I guess a generic map parameter with a default fallback could work? Not sure how messy that would become, and it would need to define a map-esque trait...
mitsuhiko commented 5 years ago

@arcnmx a lot of the serde ecosystem completely breaks sadly by turning on features. For instance serde-json's arbitrary_precision support completely breaks one of our projects. Given that this is how the ecosystem works at the moment I'm not super concerned about turning on this feature here having worse repercussions.

dtolnay commented 5 years ago

I think this breakage is worse than what happens around arbitrary_precision. It would be better to solve this the way @arcnmx asked, using an opaque map type like how serde_json's preserve_order works.

Separately, are there use cases that require this to be a map at all? Otherwise Vec<(Value, Value)> could be a more appropriate underlying type.

arcnmx commented 5 years ago

That's a good point - or alternatively, would it be appropriate to make indexmap the only option if the overhead is comparable?