Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

# fmt: skip ignored in if conditionals #3682

Open
metov opened this issue May 7, 2023 · 3 comments · May be fixed by #3978
Open

# fmt: skip ignored in if conditionals #3682

metov opened this issue May 7, 2023 · 3 comments · May be fixed by #3978
Labels
F: fmtskip fmt: skip implementation T: bug Something isn't working

Comments

@metov
Copy link

metov commented May 7, 2023

Describe the bug

# fmt: skip does not work in if conditionals.

To Reproduce
I expect the following code to not be reformatted, due to the skip directive. It is.

if True: print("yay") # fmt: skip

https://black.vercel.app/?version=stable&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4AC8AG9dAD2IimZxl1N_Wlws4TBexXdsrAK05TP_h68mLf6AjhWJHCRgXa5Rwd9gFE21oq9pVC8qdR56y-a_GL_VJamRio8oas6Eub07I8wP5I3sv2WdLkei-6Q30-uDRVSurPlC7LiqqMoMjKtW0_uwtGuaAAAAiskgpy3XxxYAAYsBvQEAAAblmGSxxGf7AgAAAAAEWVo%3D

Expected behavior

Since the line contains # fmt: skip, black should leave it alone.

Environment

Problem happens on Black Playground, clearly it is not env specific.

  • Black's version: 23.3.0
  • OS and Python version: Arch Linux, Python 3.10.9
@metov metov added the T: bug Something isn't working label May 7, 2023
@metov
Copy link
Author

metov commented May 7, 2023

This may be related to #3364. I'm not really familiar with how exactly black works, but I assume the skip directive applies to a single "syntactic element", and the condition is split off from the statement into a separate element before the skip is applied. Clearly it should be the other way around.

This is only a hunch though. Please do not close either of these as duplicate until there is actual evidence that these are indeed the same bug.

@JelleZijlstra JelleZijlstra added the F: fmtskip fmt: skip implementation label May 7, 2023
@treykeown
Copy link

I believe the underlying cause for my issue is related. #fmt: skip doesn't work for single-line class definitions like this:

class Foo: ...  # fmt: skip

It's always reformatted for me to

class Foo:
    ...  # fmt: skip

henriholopainen added a commit to henriholopainen/black that referenced this issue Oct 26, 2023
Here we enable the backtracking to reach preceding leafs even outside the same parent. As long as they are on the same line, it should be fine, as  is supposed to target the whole line.
@tim-timman
Copy link

Just want to add another situation this bugs out, but I've verified that #3978 fixes this (so please get that merged).

For example, take this code:

# {} () [] / % & | and or ... - + .  etc as part of the "else" clause presents the bug
(
    ""
    if True else  # fmt: skip
    []
    # NOTE: this entire comment is completely GONE!
    # Doesn't matter how many rows
)

Run in Online Formatter

Incorrectly returns:

(
    ""
    if True
    else []  # fmt: skip
)

Note: The comment is gone and the if True else line was broken.

Whereas in here it causes the formatting to fail completely.

(
    ""
    if True else  # fmt: skip
    [""]  # any single element [x], {x} (not parens), or ""[x] etc. breaks given that this comment (together with "else ..." format) goes beyond the allowed line length (default: 88)
)

Run in Online Formatter

The resulting error is:

Unable to match a closing bracket to the following opening bracket: ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: fmtskip fmt: skip implementation T: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants