-
Notifications
You must be signed in to change notification settings - Fork 52
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
a slightly better version of restore_yaml_comments #61
base: main
Are you sure you want to change the base?
Conversation
Interesting! Thanks for looking into this. Would you mind adding some tests to specify the behavior you're going for here? |
I will add a test. Would it be fine if it will call only restore_yaml_comments() or you would like to call dump()? |
Cool! I guess it would be nice to keep the name, just in case anyone is depending on that function directly. |
Here it is. |
I have been playing with the code and found out that in case of merging configuration from several sources my code fails to perform correctly. Do not incorporate it to master, I am looking into this. |
This version processes "reordered" texts (e.g. a section was at the top of config_default.yaml file and dump() provides it in the bottom of it's output). Benefits:
Limitations:
IMO to overcome the limitations the only way is to parse text back to YAML object. |
"Indentation is not aligned." this one is regarding indentation of comments. |
I discovered that config.dump() throws StopIteration if my config_default.yaml has comments in the end of the file.
The reason was how original restore_yaml_comments() processed default_data.
Also, original restore_yaml_comments() would put a comment in front of ALL the keys with the same name.
And if a comment was indented, the original restore_yaml_comments() ignored it.
I made some changes that fix these three problems.
It is not ideal - for example, it does not align indentation and I imagine a case where it would misplace a comment, but it improves current implementation.