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

Keep dict insertion order during dump #48

Closed
h-vetinari opened this issue May 19, 2021 · 12 comments
Closed

Keep dict insertion order during dump #48

h-vetinari opened this issue May 19, 2021 · 12 comments

Comments

@h-vetinari
Copy link
Contributor

Hey, thanks for this library!

I just followed down the rabbit-hole of figuring out what would be necessary to maintain insertion order, and basically, this is blocked on marzer/tomlplusplus#28 / marzer/tomlplusplus#62.

I just thought I'd open an issue here for someone else who might be wondering about this (and yes, I'm keen to see this resolved, even if it means fixing it on the tomlplusplus side). 🙃

@h-vetinari h-vetinari changed the title Keep dict insertion order when dumping Keep dict insertion order during dump May 19, 2021
@marzer
Copy link
Collaborator

marzer commented May 19, 2021

fixing it on the tomlplusplus side

Fixing it implies that it's broken. It's not. It disregards order on purpose, as I justify quite comprehensively in the thread you linked. If you want this 'fixed', fix it in python, as toml++ will stay the way it is for the forseeable future.

@bobfang1992
Copy link
Owner

bobfang1992 commented May 19, 2021

I'd rather not to change tomlplusplus's behaviour unless there is a compelling reason. Could you give me some reason? Why this is needed?

@bobfang1992
Copy link
Owner

Or can it be done with some params to control this behaviour instead of by default?

@marzer
Copy link
Collaborator

marzer commented May 19, 2021

@bobfang1992 there's no way to do this C++-side without nontrivial modification to TOML++. The root of the issue is that the lib uses std::map to take advantage of C++17's heterogeneous lookup, a side effect being that std::map doesn't track insertion order, and iteration is simply ordered according to key, so things get re-ordered alphabetically.

Theoretically, If I made the underlying map configurable, a user could swap it out for something that maintained insertion order, but it would also need to support heterogeneous lookup for it to compile. To my knowledge nothing like that exists (though it has been a while since I looked, to be honest).

@h-vetinari
Copy link
Contributor Author

@bobfang1992: I'd rather not to change tomlplusplus's behaviour unless there is a compelling reason. Could you give me some reason? Why this is needed?

Not least anytime you want to read & resave files that are not only machine processed - the machine doesn't care about the order, but since this is a config format that's supposed to be human-readable, a great many use-cases actually require that files are modifiable by humans (and order of entries for real-world things are semantically relevant beyond what the format spec or std::map require, so humans care about not having their order destroyed in the process).

I realized from looking at the code that this isn't possible without modifying toml++, which is why I commented there - I would suggest to focus the discussion of that in marzer/tomlplusplus#28; I only opened the issue because other people who are discovering this python-wrapper will also stumble over the same problem.

@bobfang1992
Copy link
Owner

cool thanks for the clarification. I will keep this open so people can easily spot this issue if they wanted the same thing. Seems nothing can be done on pytomlpp's side.

@h-vetinari
Copy link
Contributor Author

Seems nothing can be done on pytomlpp's side.

It's not completely impossible (as hardly anything is...) - py_dict_to_toml_table could keep track of insertion order and save that information in a way that it can get reused when writing, but IMO it would be much more desirable to implement this on the toml++ side.

@bobfang1992
Copy link
Owner

closing on this as we do not plan to support it.

@pwwang
Copy link

pwwang commented May 6, 2022

Here is an reference:
https://github.com/pwwang/toml-bench#testdumpkeysorder
even though it's not in the plan

@zhihanyue
Copy link

I think preserving order is crucial for generating config file. Because the config file is for humans. For example, we want to put some important settings (such as "general", "basic") at the top of the config file. How to implement this?

@bobfang1992
Copy link
Owner

@yuezhihan Well in general in open source world there is a saying "be the change you want". I'd encourage you to take a look at the source code of this repo first, and see if we can do it here. It is a relatively simple code base and we might be able to do it here without touching the underlying tomlpp library.

@h-vetinari
Copy link
Contributor Author

I'd encourage you to take a look at the source code of this repo first, and see if we can do it here.

I did take a look at the time, and I don't think it's possible without either:

  • carrying insertion order metadata in pytomlpp (presumed undesirable)
  • fixing it in toml++ (runs into limitations of available C++ containers vs. desired performance characteristics)

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

No branches or pull requests

5 participants