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

add preserve_order feature #80

Merged
merged 4 commits into from
Jun 24, 2016
Merged

add preserve_order feature #80

merged 4 commits into from
Jun 24, 2016

Conversation

laktak
Copy link
Contributor

@laktak laktak commented Jun 15, 2016

I've added a PR for #54

linked-hash-map needs to be released (probably version = "0.0.11") before this can be merged.

I had to use linked_hash_map::Iter<'a, String, Value> as a replacement for btree_map::IntoIter<String, Value>. This in turn required me to clone keys/values in deserialize_enum and visit_key. I'm not sure this is the best solution.

Also since I'm new to Rust and this is my very first PR please check my code thoroughly :)

@dtolnay
Copy link
Member

dtolnay commented Jun 15, 2016

I would strongly prefer to use IntoIter with linked-hash-map. I have a pull request at contain-rs/linked-hash-map#54 that makes the backend changes required for IntoIter, and a proto-pull request at https://github.com/dtolnay/linked-hash-map/pull/1 that adds IntoIter. Let's find a way to get those into linked-hash-map before tackling #54.

@laktak
Copy link
Contributor Author

laktak commented Jun 16, 2016

OK. If you like I can update this when it's ready.

@dtolnay
Copy link
Member

dtolnay commented Jun 23, 2016

contain-rs/linked-hash-map#54 has finally been merged, now waiting on contain-rs/linked-hash-map#57.

@dtolnay
Copy link
Member

dtolnay commented Jun 23, 2016

That one merged too, waiting on a release contain-rs/linked-hash-map#56.

@@ -824,8 +856,8 @@ impl de::Deserializer for Deserializer {
Some(_) => Err(de::Error::invalid_type(de::Type::Map)),
None => visitor.visit(VariantDeserializer {
de: self,
val: Some(value),
variant: Some(Value::String(variant)),
val: Some(value.clone()),
Copy link
Member

Choose a reason for hiding this comment

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

These clones are no longer required.

@dtolnay
Copy link
Member

dtolnay commented Jun 24, 2016

I released v0.7.2 with this feature.

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

Successfully merging this pull request may close these issues.

None yet

2 participants