Skip to content

a slightly better version of restore_yaml_comments#61

Open
hebote wants to merge 5 commits into
beetbox:mainfrom
hebote:dump_fix
Open

a slightly better version of restore_yaml_comments#61
hebote wants to merge 5 commits into
beetbox:mainfrom
hebote:dump_fix

Conversation

@hebote

@hebote hebote commented Aug 27, 2019

Copy link
Copy Markdown

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

sampsyo commented Aug 27, 2019

Copy link
Copy Markdown
Member

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

@hebote

hebote commented Aug 27, 2019

Copy link
Copy Markdown
Author

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

sampsyo commented Aug 27, 2019

Copy link
Copy Markdown
Member

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

@hebote

hebote commented Aug 29, 2019

Copy link
Copy Markdown
Author

Here it is.

@hebote

hebote commented Aug 29, 2019

Copy link
Copy Markdown
Author

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

hebote commented Aug 29, 2019

Copy link
Copy Markdown
Author

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

hebote commented Aug 29, 2019

Copy link
Copy Markdown
Author

"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