[3608] Fix using multiple fmt:skip pragmas in a single block

[henriholopainen]

Description

This PR contains some refactoring to comments.py, as the code was not the cleanest and comments/naming were not up to date. This also fixes bugs with using multiple fmt: skip pragmas inside a single block as originally reported in 3608, and another problem with skipping statements on the same line as block open, as reported in 3682.
Due to the refactoring, this PR is in small commits, so it's easier to review the changes piece by piece.

Fixes #3608
Fixes #3682

Checklist - did you ...

• Add an entry in CHANGES.md if necessary?
• Add / update tests if necessary?

[github-actions]

diff-shades results comparing this PR (58e011b) to main (46be1f8). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 1 projects & 1 files changed / 3 changes [+1/-2]       │
│                                                        │
│ ... out of 2 511 581 lines, 11 768 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

[henriholopainen]

Apparently one line in Hypothesis would be changed due to this. Are bug fixes also supposed to go behind --preview?

[JelleZijlstra]

Apparently one line in Hypothesis would be changed due to this. Are bug fixes also supposed to go behind --preview?

Part of the point of the stable style is so we don't have to decide whether something is a bug or a feature. When you upgrade from one version of Black 23.* to another, you shouldn't see any formatting changes on already-formatted code. However, in this case it looks like the change is made only in preview style, so you're fine!

I'll look at the rest of the diff over the next few days.

[henriholopainen]

After looking at the format skipping code for a while now, it is starting to feel like modifying the AST by rewriting nodes to standalone comments might have not been the optimal approach and PR #3610 might have been a step into problems in the future. Format skipping could instead be a property of the node and leave the tree intact. Then it would be the responsibility of transform_line to skip lines that contain nodes that have format skipping enabled.

There might be something I still don't understand about the code, but I think giving this change a shot could tell if it's feasible quite fast. Or if you @JelleZijlstra have some insight why the current approach should not be changed, please let me know! 🙏

[JelleZijlstra]

I'm open to alternative approaches for implementing # fmt: skip. We'll just have to try it and see if it works.

Is there a concrete problem that the current approach can't solve?

[henriholopainen]

I'm open to alternative approaches for implementing # fmt: skip. We'll just have to try it and see if it works.

Is there a concrete problem that the current approach can't solve?

I guess the current approach can solve cases, but it leads to the transformation code being contaminated with checks for standalone comment created by fmt: skip. Case example (from issue 3438):

if (
    self.previous_line
    and self.previous_line.is_import
    and not current_line.is_import     <---
    and not current_line.is_fmt_pass_converted(first_leaf_matches=is_import)    <---
    and depth == self.previous_line.depth
):

This would be relevant to

import foo
import bar  # fmt: skip

If the nodes are not transformed to standalone comments, we wouldn't have to check for it everywhere we might be interested in the fact. Currently there probably are edge cases where the code doesn't check both for node type and standalone comment containing that node type, but those are not tested against and nobody has reported them. And if they surface, I don't think fixing them with the current approach will be a battle that can be won.

[henriholopainen]

@JelleZijlstra should this PR be split into smaller ones to make reviewing more inviting?

[JelleZijlstra]

It's probably fine as is, I'll need a bit of time to review it though. (And I'd welcome reviews from anyone else following along.)