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

Behavior of # fmt: skip in the presence of other comments/pragmas #3330

Closed
e-gebes opened this issue Oct 13, 2022 · 9 comments
Closed

Behavior of # fmt: skip in the presence of other comments/pragmas #3330

e-gebes opened this issue Oct 13, 2022 · 9 comments
Labels
F: fmtskip fmt: skip implementation

Comments

@e-gebes
Copy link

e-gebes commented Oct 13, 2022

I have this code:

def function():
    from . import something_long_long_long_long  # pylint: disable=import-outside-toplevel

black formats it to:

def function():
    from . import (
        something_long_long_long_long,
    )  # pylint: disable=import-outside-toplevel

This breaks the Pylint pragma, it does not work anymore on the line where black has put it. So, I played around with the pylint and black pragmas to get it working:

def function():
    # option 1: move the pylint pragma after black did its work
    from . import (  # pylint: disable=import-outside-toplevel
        something_long_long_long_long,
    )

    # option 2: try fmt: skip at the end of the line
    from . import something_long_long_long_long  # pylint: disable=import-outside-toplevel  # fmt: skip

    # option 3: try fmt: skip at the beginning of the comment
    from . import something_long_long_long_long  # fmt: skip  # pylint: disable=import-outside-toplevel

    # option 4: use the off/on pragmas
    # fmt: off
    from . import something_long_long_long_long  # pylint: disable=import-outside-toplevel
    # fmt: on

All those 4 options are fine for Pylint. Options 1 and 4 are fine for Black. Black ignores the # fmt: skip pragmas in options 2 & 3.
Option 4 is my current workaround, though it's not quite nice having to use two pragmas on additional lines.

In my opinion, black should at least work with the skip pragmas as in option 2 (at the end of the line). Maybe also option 3 (the first part of the comment).
In the current documentation there is this wording:

[Black] doesn’t reformat lines that end with # fmt: skip

This suggests that option 2 should work, but it doesn't. The documentation should be made clearer, especially if this issue would not be fixed.

An idea is also to exclude Pylint pragmas (and possibly pragmas from other tools) from calculating the line length in black. Pylint does this with its own pragmas (there is also a Pylint line length check), which makes sense because pragmas are usually not something that one needs to read very often, hence it's kinda acceptable to exceed the line limit.

(Related to #3329, which is a matter of style, this here is about interoperability with other tools.)

@e-gebes
Copy link
Author

e-gebes commented Oct 13, 2022

I have here another example which illustrates that using the off/on pragmas ("option 4" in my original posting) has drawbacks in certain situations:

class A:
    def method(self):
        # fmt: off
        for value in self.plugin.job_parameters._data.values():  # pylint: disable=protected-access
            # code here would not be formatted by black
            ...
        # fmt: on
        # formatting can be turned on again earliest after the whole for-loop block

An alternative here could be to refactor further, this could make the use of # fmt: off/on obsolete, or at least make it possible to use it around a sole statement, and not around a whole block:

class A:
    def method(self):
        # fmt: off
        workaround_name_might_be_long_too = self.something_long._data  # pylint: disable=protected-access
        # fmt: on
        for value in workaround_name_might_be_long_too.values():
            # this block now gets properly formatted

This shall illustrate that it is useful to have a single-line pragma (for whatever tool), as well as that it would be useful if those pragmas could be used with pragmas of other tools on the same line. Otherwise, it would lead here and there to annoying workarounds.

What do you think about this: The # fmt: skip pragma should also work if it is embedded in the comment, but properly separated with # and two spaces. E.g.

statement  # something before  # fmt: skip

statement  # fmt: skip  # something after

statement  # before  # fmt: skip  # after

Edit: I think there is the common approach to separate different pragmas via a semicolon, so, what about checking whether any partition of the comment - divided either by ; or #, equals fmt: skip?

@yilei
Copy link
Contributor

yilei commented Oct 13, 2022

Related: #2843 (comment) where I proposed a few options related to how Black can statistically improve pragma comments related line wrappings.

@e-gebes
Copy link
Author

e-gebes commented Oct 13, 2022

I have another interesting example. It is about and odd behavior between black, isort, and pylint.

There is this code, which is fine for isort and also Pylint. The Pylint pragma works, and isort does not reformat the statement - which apparently means that it does not take the Plyint pragma for line length into account:

from my_plugins import abc_plugin as abc_plugin_with_odd_name  # pylint: disable=unused-import

But for black the line is too long, and it formats it into:

from my_plugins import (
    abc_plugin as abc_plugin_with_odd_name,
)  # pylint: disable=unused-import

This breaks Pylint, because the pragma does not work anymore on the line where it was put.
Running isort again will revert the thing back to the original code. The only way out is some user interaction. Let's put the Pylint pragma where it would work:

from my_plugins import (  # pylint: disable=unused-import
    abc_plugin as abc_plugin_with_odd_name,
)

This works now for Pylint and for black! But not for isort... it would reformat it again back to the original code (into a single line).
Let's try to add also a pragma for isort:

from my_plugins import (  # pylint: disable=unused-import  # isort:skip
    abc_plugin as abc_plugin_with_odd_name,
)

This works now for black, isort, and Pylint. (It does not matter if the two pragmas are separated with a # or a ;.)

By the way, in this case it would not be possible to use black's fmt: off/on multiline pragmas (outlined in my earlier postings as option 4), because isort would put those comments at different places, and would not keep it exactly around the one import statement.

Implementing option 2 (the single line fmt: skip pragma, see above) would improve the situation, one would be able to find a solution easier than with playing around where to put which pragma of which tool.. In my opinion, this would still only be a workaround - if isort supports ignoring Pylint pragmas for the line length, and black is apparently interested in smooth tool interoperability (see the docs!), then I think also black should ignore pragmas for the line length. Edit: I think this changed somewhere from isort 4.3.21 to 5.10.1.. in v5 also isort does break overlong pylint pragmas - which is what black does. I did not find a related isort issue on Github.

If you need any more information, please just ask for it.

I was using

black 22.10.0 (default config)
isort 5.10.1 (with force_single_line=True and the compat. options)
pylint 2.14.3

@JelleZijlstra
Copy link
Collaborator

This issue goes into a bit of extraneous detail, but I think there's a fairly clear bug. Given this code:

x+1 # fmt: skip
x+2 # fmt: skip # hello
x+3 # hello # fmt: skip

Black will currently leave 1 alone, but format 2 and 3 (adding spaces around the +). I agree that at least one of 2 and 3 should be left alone, probably both. I would accept a PR making that change.

@ricardo-dematos
Copy link

This issue goes into a bit of extraneous detail, but I think there's a fairly clear bug. Given this code:

x+1 # fmt: skip
x+2 # fmt: skip # hello
x+3 # hello # fmt: skip

Black will currently leave 1 alone, but format 2 and 3 (adding spaces around the +). I agree that at least one of 2 and 3 should be left alone, probably both. I would accept a PR making that change.

2 means why we are skipping formatting this line.
3 means we must skip formatting this line, because the comment explaining the code breaks formatting.

Both should honor the directive, IMHO.

@jamesbraza
Copy link
Contributor

x+1 # fmt: skip
x+2 # fmt: skip # hello
x+3 # hello # fmt: skip

Black will currently leave 1 alone, but format 2 and 3 (adding spaces around the +). I agree that at least one of 2 and 3 should be left alone, probably both. I would accept a PR making that change.

From reading around (and if interpreting correctly), it looks like an extreme version of case 3 was not accepted in #3450.

To share an opinion for case 2, I am a fan of allowing one to document reasoning on the same line (less overall lines, reasoning is closest to disable):

x+2  # fmt: skip  # Some reasoning for skipping

# Or

# fmt: off  # Some reasoning for turning off
x+2
# fmt: on  # Some reasoning for turning on

I think @ricardo-dematos had the same opinion on case 2 in his comment above. Would be cool to allow this

@JelleZijlstra
Copy link
Collaborator

#3450 turned out to be a different request to ignore comments for the purpose of line length, it's not relevant here as this issue is only about # fmt: skip.

I think we should leave both cases (2) and (3) from my message alone, which is also what @ricardo-dematos argued for above.

If you're interested in seeing this behavior in Black, please send a pull request to implement it.

@Nebucatnetzer
Copy link

I wanted to silence pylint and prevent from formatting this line:

import pdb; pdb.set_trace()  # fmt: skip  # pylint disable=all

As mentioned mixing the pragmas doesn't work however in other issues I've found the following workaround.
Both pylint and black ignore this line.

__import__("pdb").set_trace()

@JelleZijlstra
Copy link
Collaborator

Duplicate of #2213

@JelleZijlstra JelleZijlstra marked this as a duplicate of #2213 Oct 23, 2023
@JelleZijlstra JelleZijlstra closed this as not planned Won't fix, can't repro, duplicate, stale Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: fmtskip fmt: skip implementation
Projects
None yet
Development

No branches or pull requests

6 participants