Fix bug where # fmt: skip is not being respected with one-liner functions

[Pedro-Muller29]

Description

Fixes #3682
Fixes #4535

The fix involves modifying the algorithm that determines which nodes to ignore in the _generate_ignored_nodes_from_fmt_skip function. Specifically, the adjustment applies to cases where the # fmt: skip comment is not immediately after the colon in an if, while, def, class, etc.

Previous Algorithm

The previous algorithm for selecting ignored nodes worked as follows:

1. Add the previous sibling of the current node to the list of ignored nodes.
2. Set the current node to its previous sibling.
3. Stop if the current node has no previous sibling or its previous sibling contains a \n in its prefix.

This approach worked well when the node with the comment existed at a tree level that covered the entire line. However, it failed in cases where this assumption was invalid. For example, consider the tree produced for the code in #4535:

Node(funcdef)
├── Leaf(NAME, 'def')   ─┐
├── Leaf(NAME, 'foo')    │
├── Node(parameters)     │
 │   ├── Leaf(LPAR, '(') │
 │   └── Leaf(RPAR, ')') ┣── This level is never reachable, so they won't be ignored by # fmt: skip.
├── Leaf(COLON, ':')     │
└── Node(simple_stmt)   ─┘
    ├── Leaf(STANDALONE_COMMENT, 'return "mock"  # fmt: skip') <-- The algorithm begins here.
    └── Leaf(NEWLINE, '\n')
Leaf(ENDMARKER, '')

As shown, only the content of the return "mock" # fmt: skip would be ignored.
This can be tested with the following input (note the single quotes):

def foo(): return 'mock' # fmt: skip

The resulting output would be:

def foo():
    return 'mock' # fmt: skip

Here, the single quotes are protected by the skip statement.

New Algorithm

The new algorithm traverses the tree differently, ensuring it can visit all leaves in the order they appear in the source code (but in reverse). For the example tree:

1. The traversal moves from the comment to the colon (:), then to the RPAR, LPAR, and so on.
2. It stops when encountering a NEWLINE or INDENT node.
3. All visited leaves are ignored by # fmt: skip, expect the NEWLINE or INDENT node.

This approach ensures all relevant nodes are included in the ignored list.

Checklist - did you ...

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

[Pedro-Muller29]

I’d also like to mention that I included this in the Stable Style, as I considered it more of a bug fix than a new feature.

However, I’m not yet fully familiar with the criteria for what qualifies as Stable versus Preview Style, and I couldn’t find much information about this in the documentation. Any guidance would be greatly appreciated.

[MeGaGiGaGon]

Suggested change

	def foo(): return "mock" # fmt: skip
	def foo(): return "mock" # fmt: skip
	if True: print("yay") # fmt: skip
	for i in range(10): print(i) # fmt: skip

Looks to also fix #3682 so this adds two examples from that as test cases. The other issues not fixed by this PR in #3682 look to have been fixed by something in the 03-15-2024 release, probably #4248, so it should be good to add to the close-on-merge list for this PR along with #4535
(To do that, edit the opening description to include
Fixes #3682
Fixes #4535
)

[Pedro-Muller29]

Thanks for the review.

I added the suggested tests + some extra ones.
I also mentioned #3682 on the PR description as suggested.

[cobaltt7]

However, I’m not yet fully familiar with the criteria for what qualifies as Stable versus Preview Style, and I couldn’t find much information about this in the documentation. Any guidance would be greatly appreciated.

Typically speaking, everything should go in the preview style, the only exceptions being bug fixes for crashes and code changes that don't change code that has already been formatted by Black. This fits under the second case, so it does not need to go through the preview style if it doesn't make sense for it to.

[Pedro-Muller29]

However, I’m not yet fully familiar with the criteria for what qualifies as Stable versus Preview Style, and I couldn’t find much information about this in the documentation. Any guidance would be greatly appreciated.

Typically speaking, everything should go in the preview style, the only exceptions being bug fixes for crashes and code changes that don't change code that has already been formatted by Black. This fits under the second case, so it does not need to go through the preview style if it doesn't make sense for it to.

Thank you for the clarification!

I’ve already updated my PR to use the preview style. During this process, the test I added naturally started failing, since it was running on stable mode. I attempted to give the --preview flag to the test case, but it didn’t resolve the issue.

The test only worked when I gave it the --unstable flag, which allowed the code to enter the branch if Preview.fix_fmt_skip_in_one_liners in mode. I noticed that other PRs introducing features to the preview mode (e.g., #4498) also used the --unstable flag in their tests.

Is this the correct approach? If so, could you clarify the intended purpose of the --preview flag in test cases?