-
Notifications
You must be signed in to change notification settings - Fork 221
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
Conversation
So far this looks good to me. Take your time, of course! 👍 No need to rush things here 😄 |
@matthiasbeyer any idea when you'll have a chance to look at this? |
Sorry for the delay 😐 |
@matthiasbeyer No worries -- you've been very responsive, I just wanted to make sure it wasn't forgotten 😄 |
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 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 😄 ) |
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? |
I never squash merge because that kills history. So yes, please, rebase this 😊 |
@matthiasbeyer friendly ping after the rebase -- I didn't remove any history except for the formatting commit |
Thanks for your contribution! I hope you come back for more! ❤️ 😆 |
I didn't catch that when merging PR rust-cli#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 rust-cli#217 from dlo9/master")
I didn't catch that when merging PR rust-cli#217. Fixes: be82af2 ("Rename MapImpl to Map") Fixes: 0d3a5c3 ("Merge pull request rust-cli#217 from dlo9/master") Signed-off-by: Matthias Beyer <[email protected]>
I didn't catch that when merging PR rust-cli#217. Fixes: be82af2 ("Rename MapImpl to Map") Fixes: 0d3a5c3 ("Merge pull request rust-cli#217 from dlo9/master") Signed-off-by: Matthias Beyer <[email protected]>
Fixes #216 by using
LinkedHashMap
instead ofHashMap
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 aLinkedHashMap
. Once that crate adds support for Serde 1.0, the feature can be re-enabled and everything should work. There is an alternativehjson
serde implementation, but it makes a design descision incompatible with use in this projectMerged configs will be order-preserving within each config -- that is, given the following configs in order:
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:
TODOs:
indexmap
instead ofLinkedHashMap
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 usingLinkedHashMap
because I wasn't aware of the other crate, but would like to useindex-map
before marking the PR readyHashMap
->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