Skip to content

fix(fmt: skip): respect skip directive on multi-line compound statements#5114

Open
armorbreak001 wants to merge 1 commit intopsf:mainfrom
armorbreak001:fix/fmt-skip-multiline-compound-stmt
Open

fix(fmt: skip): respect skip directive on multi-line compound statements#5114
armorbreak001 wants to merge 1 commit intopsf:mainfrom
armorbreak001:fix/fmt-skip-multiline-compound-stmt

Conversation

@armorbreak001
Copy link
Copy Markdown

Summary

Fixes #5112

When # fmt: skip is placed on the colon line of a multi-line class (or def/if) statement, Black still reformatted it — collapsing multi-line type parameters onto a single line.

Root cause: In _generate_ignored_nodes_from_fmt_skip(), line 647 unconditionally sets prev_sibling = parent.prev_sibling when the leaf (NEWLINE) has no previous sibling. This causes the function to take the per-sibling traversal path (which stops at any leaf containing a newline in its prefix) instead of the suite-sibling collection path (the elif branch at line 738, which correctly collects ALL previous siblings of the suite node).

For a multi-line class like:

class Foo(Base[\
    "A", "B"\
]):  # fmt: skip

The traversal starts at : and collects ], ), : — then hits ] whose prefix contains \n (end of line), stopping early. The entire class header (class Foo(Base[, type params) is never marked as ignored, so Black reformats it.

Fix: Add parent.type != syms.suite condition on line 646 so that suite NEWLINEs fall through to the elif branch that properly collects all statement header nodes.

Test plan

  • Reproduced issue # fmt: skip regression #5112 with the exact example from the bug report — now left unchanged
  • Verified existing # fmt: skip cases still work (assignments, one-line compounds, etc.)
  • All 210 format tests pass
  • All 19 fmtskip format tests pass
  • All 3 skip-related unit tests pass

When  is placed on the colon line of a multi-line class
(or def/if) statement, Black still reformatted it because the sibling
traversal in _generate_ignored_nodes_from_fmt_skip stopped at the first
leaf containing a newline in its prefix (e.g., the closing  of a
multi-line type parameter list).

Root cause: line 646-647 unconditionally set prev_sibling to
parent.prev_sibling for NEWLINE leaves with no prev_sibling, causing
the function to take the per-sibling traversal path (which stops at
newlines) instead of the suite-sibling collection path (which correctly
collects ALL previous siblings of the suite node).

Fix: skip the prev_sibling override when parent is a suite, so the
elif branch that handles compound statements is reached instead.
@pekkaklarck
Copy link
Copy Markdown

Thanks for taking a look at this and for the quick fix @armorbreak001! I don't know the rules of this project, but in general I believe it would be a good idea to add a new test that reproduces the bug and is fixed by this change.

@cobaltt7
Copy link
Copy Markdown
Collaborator

Thanks @armorbreak001! Yes, please do add a new test case, as well as a changelog entry.

@github-actions
Copy link
Copy Markdown
Contributor

diff-shades results comparing this PR (dded6c6) to main (8dcc486):

--preview style: no changes

--stable style: no changes


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

# fmt: skip regression

3 participants