Keys are shuffled when loading and saving a INI file

[vthib]

Hello, thanks for your work on this crate.

The sections and the keys are stored in a HashMap, in which ordering is not guaranteed. That causes sections and keys to be shuffled around when saving an INI file that has been loaded, which isn't ideal.

The Python configparser module uses an OrderedDict to avoid this issue. In Rust, an easy solution would be to replace the HashMaps with BTreeMaps to guarantee an ordering, without depending on anything but standard Rust. That wouldn't necessarily be in the same order as loaded, but it would at least prevent a shuffle of the keys on every load/write.

If you are ok with this change, I can write a PR for it.

[QEDK]

which isn't ideal.

Before I commit to this change, can you explain why? I am not confident that ordering is necessary.

[vthib]

This is mainly a UX issue. For any INI file that is possibly viewed and/or modified by a user, having shuffled keys when viewing and browsing the file is not pleasant. A user that read or modified the file will be very confused when re-opening the file later if the file was saved again by the program (which happens frequently, due to default values, new keys added, etc).

The rust HashMap also seeds the map with some randomness, which causes a big shuffle on any load/save, not just a few reordering of some keys, making the UX worse.

Ideally, keeping the reading/insertion order is the best to preserve the file and allow grouping fields together, but that would be harder to implement (at least without using an external dependency), and using BTreeMap at least gives an ordering much more natural and pleasant to read for an end user.

[vthib]

Another possible option to avoid breaking the public API is to iterate on the sorted sections & keys when writing the file, and activate this behavior in the IniDefaults

[vthib]

I have a PR ready for this (adding a sort_on_write flag in InitDefaults), as well as a PR ready for #18 . Would you be interested in those changes?

[QEDK]

I have a PR ready for this (adding a sort_on_write flag in InitDefaults), as well as a PR ready for #18 . Would you be interested in those changes?

My primary concern is that if someone is already using IniDefault in an incompatible way, they would need to update their code so this will be a breaking change even if we preserve the behaviour in Ini itself.

If we do change the current behaviour, I would want something that adds a meaningful feature without breaking anything, or adds an external crate (in our case indexmap) as a feature for this purpose instead. The former approach would be something like this: https://stackoverflow.com/questions/39980323/are-dictionaries-ordered-in-python-3-6

[vthib]

I can push a PR adding a new optional feature to use indexmap, that way this does not forces a major version bump.

However, I think it would be useful to allow adding new fields in IniDefault without needing to bump the major version (this would have been useful here, and would be useful for other features). IMHO adding a non exhaustive marker on IniDefaults would allow to do that: https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute. That would mean adding this marker (and maybe other PRs), bumping the major version, and then the major version shouldn't need to be bumped in the future for most new features. What do you think about it?

[QEDK]

I can push a PR adding a new optional feature to use indexmap, that way this does not forces a major version bump.

However, I think it would be useful to allow adding new fields in IniDefault without needing to bump the major version (this would have been useful here, and would be useful for other features). IMHO adding a non exhaustive marker on IniDefaults would allow to do that: https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute. That would mean adding this marker (and maybe other PRs), bumping the major version, and then the major version shouldn't need to be bumped in the future for most new features. What do you think about it?

This is a good idea. We should ideally have used #[non-exhaustive] from the get-go to prevent situations like this, we will bump the version for this, let's do that, go for it!