Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for feature = preserve_order #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mitsuhiko
Copy link

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
Copy link
Owner

arcnmx commented Jan 31, 2019

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
Copy link
Author

@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
Copy link

dtolnay commented Jan 31, 2019

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
Copy link
Owner

arcnmx commented Feb 9, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants