black icon indicating copy to clipboard operation
black copied to clipboard

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

Open e-gebes opened this issue 3 years ago • 3 comments

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 https://github.com/psf/black/issues/3329, which is a matter of style, this here is about interoperability with other tools.)

e-gebes avatar Oct 13 '22 11:10 e-gebes

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?

e-gebes avatar Oct 13 '22 12:10 e-gebes

Related: https://github.com/psf/black/issues/2843#issuecomment-1232146924 where I proposed a few options related to how Black can statistically improve pragma comments related line wrappings.

yilei avatar Oct 13 '22 16:10 yilei

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

e-gebes avatar Oct 13 '22 16:10 e-gebes

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.

JelleZijlstra avatar Jul 10 '23 15:07 JelleZijlstra

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.

ricardo-dematos avatar Jul 10 '23 16:07 ricardo-dematos

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

jamesbraza avatar Jul 12 '23 19:07 jamesbraza

#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.

JelleZijlstra avatar Jul 12 '23 19:07 JelleZijlstra

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()

Nebucatnetzer avatar Sep 13 '23 14:09 Nebucatnetzer

Duplicate of #2213

JelleZijlstra avatar Oct 23 '23 15:10 JelleZijlstra