Skip to content

Use IndexMap instead of LinkedHashMap#451

Merged
dtolnay merged 3 commits intoserde-rs:masterfrom
lnicola:indexmap
Jun 7, 2018
Merged

Use IndexMap instead of LinkedHashMap#451
dtolnay merged 3 commits intoserde-rs:masterfrom
lnicola:indexmap

Conversation

@lnicola
Copy link
Copy Markdown
Contributor

@lnicola lnicola commented Jun 6, 2018

This was surprisingly easy 😕.

@lnicola
Copy link
Copy Markdown
Contributor Author

lnicola commented Jun 6, 2018

Is there a benchmark suite or something like that? I wonder if we could drop the other implementation.

@lnicola
Copy link
Copy Markdown
Contributor Author

lnicola commented Jun 6, 2018

Oh, indexmap requires 1.18.

Copy link
Copy Markdown
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks, I am okay with this. If anyone requires preserve_order on older versions we can have them send an indexmap PR to support an older version. It looks like that would just be removing their use of pub(crate) which seems easy.

Comment thread Cargo.toml
# Use LinkedHashMap rather than BTreeMap as the map type of serde_json::Value.
# Use IndexMap rather than BTreeMap as the map type of serde_json::Value.
# This allows data to be read into a Value and written back to a JSON string
# while preserving the order of map keys in the input.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rephrase this comment to include only the observable guarantees that we make about the behavior of serde_json with this feature enabled, and nothing about what the implementation does internally as we don't need to commit to a specific internal implementation.

Comment thread Cargo.toml
[dependencies]
serde = "1.0.60"
linked-hash-map = { version = "0.5", optional = true }
indexmap = { version = "1.0", optional = true }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please change the Travis configuration to add a build at indexmap's minimum supported compiler version, and remove any preserve_order builds below that compiler version.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

@dtolnay dtolnay merged commit 63facc6 into serde-rs:master Jun 7, 2018
@lnicola lnicola deleted the indexmap branch June 7, 2018 06:03
takumi-earth pushed a commit to earthlings-dev/json that referenced this pull request Jan 27, 2026
Use IndexMap instead of LinkedHashMap
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.

2 participants