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

Preserve comments for a file with only comments #183

Open
lyz-code opened this issue Sep 9, 2022 · 13 comments · May be fixed by #200
Open

Preserve comments for a file with only comments #183

lyz-code opened this issue Sep 9, 2022 · 13 comments · May be fixed by #200
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@lyz-code
Copy link
Owner

lyz-code commented Sep 9, 2022

Description

When a file has only comments, the comments are removed

Steps to reproduce

Run fix_code over:

---
# Keep comments!

Current behavior

The file is transformed to:

---
...

Desired behavior

The file is unchanged

Additional context

@rsnodgrass created the unit test in this PR which was closed due to inactivity, if you want to implement this, you can follow in that branch

@lyz-code lyz-code added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers labels Sep 9, 2022
@rsnodgrass
Copy link

This is a big issue, comments were deleted out of dozens of YAML files that explained the reasoning behind various YAML entries. This was not caught for quite some time until someone opened up a reformatted yaml file a month ago later and could not understand why documentation was missing.

Comments within yaml should be preserved, as they are often as important as the actual data in the yaml.

@lyz-code
Copy link
Owner Author

Comments are kept when the file is not empty, for example, the next snippet is left unchanged by yamlfix:

---
# this is a comment
a: 1

I understand your case, but I feel it's a corner case, as you're using a yaml file for documenting instead of holding data. I'd either suggest you to summit a PR with the fix or add a a: 1 at the end of every file.

@lyz-code lyz-code changed the title Preserver comments for a file with only comments Preserve comments for a file with only comments Sep 14, 2022
@marcules
Copy link
Contributor

The ruyaml maintainer made a comment on StackOverflow how this could be fixed: https://stackoverflow.com/a/65001415
Does not look that hard to implement, but I'm wondering, why he chose to not add this to the upstream / ruyaml project itself.

@lyz-code
Copy link
Owner Author

I'm closing this as won't fix as I don't feel this feature has a big impact. If someone want's to implement it I'm fine with reviewing it and reopening this issue.

@lyz-code lyz-code added the wontfix This will not be worked on label Dec 19, 2022
@rsnodgrass
Copy link

This is an interesting bug because sometimes 'yamlfix' gets added to GitHub workflows that are shared broadly. The "owner" of the YAML files may never even be able to make a decision whether or when yamlfix gets applied...and the workflow owner has no idea whether or not all YAML files that may be processed by their change will have comments.

This is a "destructive" bug IMO since it can destroy important information in YAML files, even if those aren't yaml fields.

@rsnodgrass
Copy link

rsnodgrass commented Dec 20, 2022

EVEN further, the very title in GitHub of 'yamlfix' is: "A simple opinionated yaml formatter that KEEPS YOUR COMMENTS!"

Someone adding yamlfix to a workflow or running it across sets of files would assume that the yaml files output would be syntactically identical AND preserve all comments, giving them no pause to run the tool. In reality, this is not the case.

@lyz-code
Copy link
Owner Author

In six months you've been the only user that has shown this need. As a maintainer I feel it's a corner case that don't reflect the general need of the program.

That being said the fix is not difficult. If you're willing to do a pull request I'm more than open to review it. Else, this will remain closed.

I'm sorry

@marcules
Copy link
Contributor

I think this issue is a special case of #123
If you look at the shebang'ed ansible file, there are actually two yaml documents in the file - a comment only document (containing only the shebang) and the rest of the ansible yaml file, separated through the document start indicator ---.

The current implementation is only handling the case for shebang, but I could see a general implementation that even would not rely on string manipulation an also not need to rely on parsing the buffer. We could add a top-level node with a uuid before parsing, run it through everything, and then just remove the line with the uuid-node -> that would prevent ruyaml from removing the comments, it would be more general as it could handle all comment-only files and multi-document yaml files, the only special case for shebang would be to prevent it from adding the document-start indicator to the first yaml document.

@lyz-code I can implement this as well, but only if you want to have it in the codebase / want to maintain such a feature.

@marcules marcules linked a pull request Dec 26, 2022 that will close this issue
2 tasks
@kbakk
Copy link

kbakk commented Sep 13, 2023

As an additional data point - we would also like to keep comments, even if it's an empty yaml document. Our use case is similar to others - entries are added as a way to document how to override some defaults.

I believe the user creating this one had some similar expectations: #255

@lyz-code Could you please reopen this one? I understand not wanting to spend time on it, but seeing that it's requested by a few people it may attract a PR contribution. 🙂

@rsnodgrass
Copy link

Tthe comments can be as valuable...or in some cases more valuable than the actual entries.

@lyz-code lyz-code reopened this Sep 14, 2023
@lyz-code lyz-code removed the wontfix This will not be worked on label Sep 14, 2023
@lyz-code
Copy link
Owner Author

Sure @kbakk , I will only accept a PR to solve this issue as long as it doesn't increase too much the complexity of maintenance

@nbari
Copy link
Contributor

nbari commented Oct 11, 2023

This breaks Ansible playbooks, for example, can't use # jinja2:lstrip_blocks: true any idea how to preserve the commenters before the --- ?

@lyz-code #263 hope this is OK, it doesn't cover maintaining all comments, but prevents breaking ansible templates.

@lyz-code
Copy link
Owner Author

@nbari your contribution is available since 1.15.0, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants