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

Preserve order of compound fields #54

Merged
merged 1 commit into from
Jul 15, 2020
Merged

Preserve order of compound fields #54

merged 1 commit into from
Jul 15, 2020

Conversation

samlich
Copy link
Contributor

@samlich samlich commented Jul 10, 2020

Keeps the order of compound fields as they are declared in the loaded file using indexmap.

Closes #42

@atheriel
Copy link
Collaborator

Since this is a breaking change and may have performance (and other) impacts, I would prefer that it be gated by a feature flag for now. For example, we could add a type definition like the following

#[cfg(feature = "preserve_order")]
type Map = IndexMap<String, Value>;
#[cfg(not(feature = "preserve_order"))]
type Map = HashMap<String, Value>;

and use that instead of the existing HashMap. This would also match the behaviour of the serde_json crate.

@samlich
Copy link
Contributor Author

samlich commented Jul 10, 2020

Sounds good, I've updated it.

I tried the benchmarks and they seem to improve in general, except for simple_player serialize as struct and complex_player serialize as struct. simple_player serialize as struct doesn't even include a map as far as I can tell, but the results are fairly consistent. It's piqued my interest, so I'll look into it a bit more.

@atheriel
Copy link
Collaborator

This looks good to me. I wouldn't pay too much mind to the benchmarks, as I'm not sure the black-box aspect is working correctly in them yet. Thanks!

@atheriel atheriel merged commit 2494c1d into PistonDevelopers:master Jul 15, 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.

Support ordered NBT compounds
2 participants