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 map ordering #217

Merged
merged 10 commits into from
Aug 20, 2021
Merged

Preserve map ordering #217

merged 10 commits into from
Aug 20, 2021

Conversation

dlo9
Copy link
Contributor

@dlo9 dlo9 commented Jul 29, 2021

Fixes #216 by using LinkedHashMap instead of HashMap to preserve the order of the map types in configs. A few things to note:

  • Ordering is not preserved when using hjson because the current dependency breaks when using a LinkedHashMap. Once that crate adds support for Serde 1.0, the feature can be re-enabled and everything should work. There is an alternative hjson serde implementation, but it makes a design descision incompatible with use in this project

  • Merged configs will be order-preserving within each config -- that is, given the following configs in order:

    # Config 1
    creator:
      name: John Smith
      username: jsmith
    # Config 2
    creator:
      email: jsmith@localhost
      name: John Smith Jr.

    The resulting config will contain settings in the order found, preserving the order within each individual config only for new settings. Updates to settings is a lower config will have no effect on ordering:

    # Merged config
    creator:
      name: John Smith Jr.
      username: jsmith
      email: jsmith@localhost

TODOs:

  • Overall, the community seems to be converging on use of indexmap instead of LinkedHashMap for this use case. This is due to some improved performance, less unsafe code, and more active development (LinkedHashMap is in maintenance mode). I initially implemented this using LinkedHashMap because I wasn't aware of the other crate, but would like to use index-map before marking the PR ready
  • This current implementation would be a breaking change, but I believe can be made a non-breaking feature by type aliasing. I would like to also make this change pre-merge
  • Check for over-replacement of HashMap -> IndexMap. I expect there are uses that don't need to be replaced because they don't impact the iteration order of the resulting config, but I haven't yet verified

@matthiasbeyer
Copy link
Collaborator

So far this looks good to me. Take your time, of course! 👍 No need to rush things here 😄

Cargo.toml Outdated Show resolved Hide resolved
@dlo9 dlo9 marked this pull request as ready for review August 3, 2021 06:23
@dlo9
Copy link
Contributor Author

dlo9 commented Aug 9, 2021

@matthiasbeyer any idea when you'll have a chance to look at this?

@matthiasbeyer
Copy link
Collaborator

Sorry for the delay 😐

@dlo9
Copy link
Contributor Author

dlo9 commented Aug 9, 2021

@matthiasbeyer No worries -- you've been very responsive, I just wanted to make sure it wasn't forgotten 😄

@matthiasbeyer
Copy link
Collaborator

So, running rustfmt and adding a seperate commit is not nice history-wise.

But, there's help! Please try the following in your local repository on your PR branch

git rebase --strategy-option=theirs --exec "cargo fmt && git add --all && git commit --amend --no-edit" <this repository>/master

then, make sure everything is fine by comparing your local branch with your pushed branch, something like this:

git diff HEAD..<this remote>/master

(btw, why are you PRing from master 😆 ?)

And if there's no diff, you can push the changes to this PR. Maybe the last commit ("Run rustfmt") is not necessary anymore... I tried this locally with you branch and the commit only contains one change, a removed import. Thus it might be good to just change the commit message 😄

Please try this! If you think you messed up I am here to help (and I have a copy of this PR locally, I can always simply roll back if you accidentially messed up 😄 )

@dlo9
Copy link
Contributor Author

dlo9 commented Aug 11, 2021

I had assumed you would squash-merge this, since the commit history in this PR has some churn and isn't terribly useful IMO. As a general rule I don't rebase after opening a PR so that I don't make the review confusing (unless coordinated with reviewers of course).

Though if you'd like me to fix up the history I'm more than happy to, would you prefer that or squashing?

@matthiasbeyer
Copy link
Collaborator

I never squash merge because that kills history. So yes, please, rebase this 😊

@dlo9
Copy link
Contributor Author

dlo9 commented Aug 20, 2021

@matthiasbeyer friendly ping after the rebase -- I didn't remove any history except for the formatting commit

@matthiasbeyer
Copy link
Collaborator

Thanks for your contribution! I hope you come back for more! ❤️ 😆

@matthiasbeyer matthiasbeyer merged commit 0d3a5c3 into mehcode:master Aug 20, 2021
matthiasbeyer added a commit to matthiasbeyer/config-rs that referenced this pull request Aug 21, 2021
I didn't catch that when merging PR mehcode#217.
CI should have catched it, but we actually never ran CI for examples.

Signed-off-by: Matthias Beyer <[email protected]>
Fixes: be82af2 ("Rename MapImpl to Map")
Fixes: 0d3a5c3 ("Merge pull request mehcode#217 from dlo9/master")
matthiasbeyer added a commit to matthiasbeyer/config-rs that referenced this pull request Aug 21, 2021
I didn't catch that when merging PR mehcode#217.

Fixes: be82af2 ("Rename MapImpl to Map")
Fixes: 0d3a5c3 ("Merge pull request mehcode#217 from dlo9/master")
Signed-off-by: Matthias Beyer <[email protected]>
matthiasbeyer added a commit to matthiasbeyer/config-rs that referenced this pull request Aug 21, 2021
I didn't catch that when merging PR mehcode#217.

Fixes: be82af2 ("Rename MapImpl to Map")
Fixes: 0d3a5c3 ("Merge pull request mehcode#217 from dlo9/master")
Signed-off-by: Matthias Beyer <[email protected]>
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.

Keep ordering of deserialized maps
2 participants