Feature/preserve comment only files #200

marcules · 2022-12-26T23:50:31Z

Fixes #183

Adds a temporary key to the document at the start and removes it again after ruyaml ran through, preventing it from removing the comments in a comment-only yaml file.

Checklist

Add test cases to all the changes you introduce
Update the documentation for the changes

coveralls · 2022-12-26T23:53:01Z

Pull Request Test Coverage Report for Build 3784079844

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

17 of 17 (100.0%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.3%) to 99.543%

Files with Coverage Reduction	New Missed Lines	%
src/yamlfix/adapters.py	1	99.64%

Totals
Change from base Build 3740603077:	0.3%
Covered Lines:	436
Relevant Lines:	438

💛 - Coveralls

marcules · 2022-12-27T00:01:02Z

@lyz-code @rsnodgrass this adds a mapping key to the document before it goes through ruyaml - meaning:

if the document already has mapping keys on the top level, this line is treated as if it were a mapping with a "none" value
if the document already has a sequences as the top level, this line is treated as if it were the name of the sequence

When it has other mappings yamlfix_document_fix_id has none value:

---
yamlfix_document_fix_id: # uuid
key: value
---
# equivalent to:
yamlfix_document_fix_id: none # uuid
key: value

When it has a top level list, yamlfix_document_fix_id is the name of said list:

---
yamlfix_document_fix_id: #uuid
- item
- item
---
# equivalent to:
yamlfix_document_fix_id: [item, item] # uuid

When the document was previously only comments, ruyaml attaches them to the "last found node" (e.g. this line):

---
yamlfix_document_fix_id: #uuid
# other comments
# comments comments

All previous tests are passing, and this should in theory work with any yaml file - but it is, admittedly, "clever" (not to say hacky 😅) - not exactly sure what happens with shorter files, that are converted to flow-style or something like that.

marcules · 2022-12-27T06:46:47Z

@lyz-code please wait on merging this until #199 is merged and this is rebased. I want to modify it a bit.

rsnodgrass · 2022-12-27T06:55:48Z

Thanks @marcules for the fix!!

lyz-code · 2022-12-27T10:47:38Z

@marcules #199 is merged! Tell me when it's ready for review

lyz-code · 2022-12-27T11:39:01Z

@marcules pull the changes from main and run make install. I've updated the dependencies versions

…ed in the Makefile for make lint

* if a yaml file contains comments only, preserve the comments (never harm the user!)

marcules · 2023-01-01T19:22:10Z

@lyz-code you can review this and pull it in now if it looks good
I have rebased it onto main and also fixed the issue in regards to flakeheaven looking into folders other than src/ tests/

lyz-code

Thanks again for your time, do you want to become a maintainer of the package? I can give you permissions, that way we can share the load of answering the users, taking care of the CI...

If you don't like the idea it's fine :)

lyz-code · 2023-01-02T11:07:37Z

docs/index.md

+Warning: You should not modify this value, if you're not sure you need to.
+
+This generates a UUID in hex-string-representation (no delimiters), which is used internally to generate a temporary top-level mapping node, where any lists or comments can be attached, so ruyaml is not removing comments, and comment-only documents can be formatted properly.
+


Do you think this is something a user would want to change? I don't feel that without knowing the internals of yamlfix they can even understand the paragraph.

If you feel it's interesting to keep the configuration, I'd move it to the bottom of the document, otherwise I'd just hardcode the value

lyz-code · 2023-01-02T11:09:19Z

src/yamlfix/adapters.py

@@ -252,6 +259,9 @@ def patch_sequence_style(key_node: Node, value_node: Node) -> None:

        self.patch_functions.append(patch_sequence_style)

+    def _seq_is_in_top_level_node(self, key_node: Node) -> bool:
+        return str(key_node.value) == f"yamlfix_{self.config.document_fix_id}"


Following the question above, can't we just use yamlfix_top_node?

lyz-code · 2023-01-02T11:14:20Z

src/yamlfix/adapters.py

+    def _fix_comment_only_files(self, source_code: str) -> str:
+        """Add a mapping key with an id to the start of the document\
+            to preserve comments."""
+        fixed_source_lines = []


Now we're running _fix_comment_only_files and _restore_comment_only_files by default, which slows down the program as it needs to iterate through the source code 2 times.

I'd only run these functions if the config is set.

lyz-code · 2023-01-02T11:20:57Z

src/yamlfix/adapters.py

+            fixed_source_lines.append(line)
+            if line.startswith("---"):
+                has_start_indicator = True
+                fixed_source_lines.append(yamlfix_document_id_line)


For performance reasons it would be nice if we could stop the iteration on the file source as soon as we've done the required changes. Same applies at the _restore_comment_only_files.

Maybe reformulate it so that once the condition is met it injects the required line and exits.

Now that I see it, although this is a good idea, it may need a non-trivial refactor where the fixers tweak directly the parts of the code from self. instead of receiving the source as an argument and returning as a return value.

If you don't feel like doing this now we can open an issue to track it for the future

marcules force-pushed the feature/preserve-comment-only-files branch from 3ab4d87 to c36d86e Compare December 29, 2022 16:13

marcules and others added 4 commits January 1, 2023 20:10

fix: run flakeheaven on the same files and folders which are configur…

73dd41e

…ed in the Makefile for make lint

for comment only yaml files, preserve the comments

ca4fe36

* if a yaml file contains comments only, preserve the comments (never harm the user!)

feat: don't erase comment-only yaml documents

0cfd930

docs: Update docs and explain usage for document_fix_id

86cac79

marcules force-pushed the feature/preserve-comment-only-files branch from c36d86e to 86cac79 Compare January 1, 2023 19:10

lyz-code requested changes Jan 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/preserve comment only files #200

Feature/preserve comment only files #200

marcules commented Dec 26, 2022 •

edited

coveralls commented Dec 26, 2022 •

edited

marcules commented Dec 27, 2022

marcules commented Dec 27, 2022

rsnodgrass commented Dec 27, 2022

lyz-code commented Dec 27, 2022 •

edited

lyz-code commented Dec 27, 2022

marcules commented Jan 1, 2023 •

edited

lyz-code left a comment

lyz-code Jan 2, 2023

lyz-code Jan 2, 2023

lyz-code Jan 2, 2023

lyz-code Jan 2, 2023

		Warning: You should not modify this value, if you're not sure you need to.

		This generates a UUID in hex-string-representation (no delimiters), which is used internally to generate a temporary top-level mapping node, where any lists or comments can be attached, so ruyaml is not removing comments, and comment-only documents can be formatted properly.

Feature/preserve comment only files #200

Are you sure you want to change the base?

Feature/preserve comment only files #200

Conversation

marcules commented Dec 26, 2022 • edited

Checklist

coveralls commented Dec 26, 2022 • edited

Pull Request Test Coverage Report for Build 3784079844

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

marcules commented Dec 27, 2022

marcules commented Dec 27, 2022

rsnodgrass commented Dec 27, 2022

lyz-code commented Dec 27, 2022 • edited

lyz-code commented Dec 27, 2022

marcules commented Jan 1, 2023 • edited

lyz-code left a comment

Choose a reason for hiding this comment

lyz-code Jan 2, 2023

Choose a reason for hiding this comment

lyz-code Jan 2, 2023

Choose a reason for hiding this comment

lyz-code Jan 2, 2023

Choose a reason for hiding this comment

lyz-code Jan 2, 2023

Choose a reason for hiding this comment

marcules commented Dec 26, 2022 •

edited

coveralls commented Dec 26, 2022 •

edited

lyz-code commented Dec 27, 2022 •

edited

marcules commented Jan 1, 2023 •

edited