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

Use indexmap to preserve order (optional) #172

Merged
merged 5 commits into from Jun 10, 2019
Merged

Conversation

torkleyy
Copy link
Contributor

@torkleyy torkleyy commented Jun 6, 2019

Introduces an optional indexmap feature which uses IndexMap in Value::Map to preserve the order of deserialized elements.

Fixes #129

Copy link
Collaborator

@kvark kvark left a comment

If I understand correctly, this would only preserve the order of Value types but not regular maps?

#[cfg(not(feature = "indexmap"))]
type MapInner = std::collections::BTreeMap<Value, Value>;
#[cfg(feature = "indexmap")]
type MapInner = indexmap::IndexMap<Value, Value>;
Copy link
Collaborator

@kvark kvark Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we just typedef type Map = indexmap::IndexMap<..> instead of wrapping it?

Copy link
Contributor Author

@torkleyy torkleyy Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll end up with a non-additive feature. BTreeMap and IndexMap have different properties (traits, methods, etc.).

Also, APIs accepting BTreeMap would break once indexmap is enabled.

@torkleyy
Copy link
Contributor Author

torkleyy commented Jun 6, 2019

If I understand correctly, this would only preserve the order of Value types but not regular maps?

Exactly. If one wants to preserve the order elsewhere, they should use IndexMap there, too.

@torkleyy torkleyy requested a review from kvark Jun 7, 2019
kvark
kvark approved these changes Jun 7, 2019
Copy link
Collaborator

@kvark kvark left a comment

I'm not feeling super comfortable reviewing the Value mode, since I don't use it.
You have my stamp though :)

@torkleyy
Copy link
Contributor Author

torkleyy commented Jun 10, 2019

Thanks for the review!
bors r+

bors bot added a commit that referenced this issue Jun 10, 2019
172: Use indexmap to preserve order (optional) r=torkleyy a=torkleyy

Introduces an optional `indexmap` feature which uses `IndexMap` in `Value::Map` to preserve the order of deserialized elements.

Fixes #129 

Co-authored-by: Thomas Schaller <torkleyy@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jun 10, 2019

Build succeeded

@bors bors bot merged commit 84b88bc into ron-rs:master Jun 10, 2019
2 checks passed
@torkleyy torkleyy deleted the idxm branch Jun 10, 2019
@CAD97 CAD97 mentioned this pull request May 1, 2020
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.

2 participants