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

[Pedro-Muller29]

trafficstars

Description

Fixes https://github.com/psf/black/issues/3682 Fixes https://github.com/psf/black/issues/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 ...

• [x] Add an entry in CHANGES.md if necessary?
• [x] Add / update tests if necessary?
• [x] 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.

[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?

[JelleZijlstra]

The idea of the stable style is that if you have a codebase formatted with Black (say) 25.1, and you upgrade to Black 25.2, nothing should change. This is useful so that users can upgrade Black and see bugfixes without having to commit reformatting. If this change only affects how Black formats code that wasn't previously formatted by Black.

Putting --preview in a test should be enough. --unstable also enables features in the UNSTABLE_FEATURES set, but shouldn't affect other preview features.

[cobaltt7]

Can you run tox -e generate_schema? This will regenerate the schema for the configuration, which is changed by adding new preview features. That will fix CI failing.

[Pedro-Muller29]

Can you run tox -e generate_schema? This will regenerate the schema for the configuration, which is changed by adding new preview features. That will fix CI failing.

Thanks for the response, I wasn't sure how to proceed. I searched in the docs but I couldn't find anything that related to the CI error. Is there any other places that contains this info? (I searched on the ReadTheDocs page in the development section).

Thanks in advance.

[Pedro-Muller29]

Now the CI seems to be failing because of the deprecation of the upload-artifact v3 action in the .github/workflows/diff_shades.yml workflow, as can be seen here.

[MeGaGiGaGon]

@Pedro-Muller29 I just fixed that in #4576, @JelleZijlstra could you re-run those two?