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

a slightly better version of restore_yaml_comments #61

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

hebote
Copy link

@hebote hebote commented Aug 27, 2019

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.

@sampsyo
Copy link
Member

sampsyo commented Aug 27, 2019

Interesting! Thanks for looking into this. Would you mind adding some tests to specify the behavior you're going for here?

@hebote
Copy link
Author

hebote commented Aug 27, 2019

I will add a test. Would it be fine if it will call only restore_yaml_comments() or you would like to call dump()?

@sampsyo
Copy link
Member

sampsyo commented Aug 27, 2019

Cool! I guess it would be nice to keep the name, just in case anyone is depending on that function directly.

@hebote
Copy link
Author

hebote commented Aug 29, 2019

Here it is.

@hebote
Copy link
Author

hebote commented Aug 29, 2019

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.

@hebote
Copy link
Author

hebote commented Aug 29, 2019

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:

  • No StopIteration exceptions.
  • Indented comments supported.

Limitations:

  • In case of several keys with the same name the comment will appear with the first occurrence only.
  • Indentation is not aligned.

IMO to overcome the limitations the only way is to parse text back to YAML object.

@hebote
Copy link
Author

hebote commented Aug 29, 2019

"Indentation is not aligned." this one is regarding indentation of comments.
E.g. a one-space indentation is fully valid for YAML and dump() uses 4 space indentation.

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.

2 participants