Fix block scalar mangling bug #231

[wrouesnel]

The regex based parsing for fixing comments was breaking block scalars. By using the ruyaml round trip handler, instead the comment formatting now can correctly identify block-scalars and avoid mangling them.

Resolves #231

Checklist

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

[coveralls]

Pull Request Test Coverage Report for Build 4628587345

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

• 61 of 62 (98.39%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.2%) to 99.634%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/yamlfix/adapters.py	52	53	98.11%

Totals
Change from base Build 4595856345:	-0.2%
Covered Lines:	545
Relevant Lines:	547

---

💛 - Coveralls

[lyz-code]

Thank you for the quick PR @wrouesnel, I'm a bit concerned with the duplication of work though which will make the code slower.

If you find a way to fix this without that duplication I'll be happy to merge this

[lyz-code]

If fixed_source == source I think it will be cleaner to use result == source. If they are different I was not able to tell them apart :S

[lyz-code]

I don't like the re-running of previous fixers, can't you fix comments before so that we don't have to rerun everything?

I think it is really bad from the point of view of the performance of the tool to do this.

[lyz-code]

Looking at the code, weren't you able to tweak the existing _fix_comments function to correctly handle the case instead of rewriting it?

[lyz-code]

Check my comment on this issue in case it might help simplify the pull request

[wrouesnel]

The problem with solving this issue is that the fixer in question takes a line-oriented approach to processing the files, and can't handle multiple lines.

The specific issue is here -

yamlfix/src/yamlfix/adapters.py

Line 584 in 51410d6

# Comment in the middle of the line, but it's not part of a string

            # Comment in the middle of the line, but it's not part of a string
            if (
                config.comments_min_spaces_from_content > 1
                and " #" in line
                and line[-1] not in ["'", '"']
            ):
                line = re.sub(r"(.+\S)(\s+?)#", rf"\1{comment_start}", line)

This code has no way to know if it's part of a string or not, because it can't know if it's part of a block, which has a number of different possible presentations.

I did try several ideas for detecting block-scalar membership, but when you get right down to it that's just re-implementing a half-baked YAML parser when there's a much better YAML parser already out there.

IMO it's more important for the fixer to be correct (i.e. not change key values) rather then fast, but I can't see a way to handle this otherwise (which isn't just trying to half-bake a YAML parser.

[lyz-code]

I'm sorry but I still feel that this PR is adding a lot of complexity to the code only to fix #231. I don't see myself maintaining this code. Why don't you take a simpler approach to have another fixer run after the comments are run that iterates over the source code and:

• If the line contains a comment that starts and ends with #:
  • Checks if the following lines start and end with #
  • Get the length of the longest line
  • Pad the rest of lines to match the desired length