flake8 #noqa comments get moved a place where they're ignored

[shish]

Operating system: Linux
Python version: 3.6
Black version: 18.4a5
Does also happen on master: ?

Using a long list of typing imports with #noqa because my code is py2 compatible so I'm using the comment syntax for type hints

original code

from typing import Dict, List, Set, ...  # noqa

becomes

from typing import (
    Dict,
    List,
    Set,
    ...,
)  # noqa

but flake8 wants

from typing import (  # noqa
    Dict,
    List,
    Set,
    ...,
)

[zsol]

You can work around this by doing

from typing import (
    Dict,  # noqa
    List,  # noqa
    ...,
)

[ambv]

Shish, Black doesn't understand how noqa comments are different from other comments. Even if it did, that would be a heuristic at best since any part of the bracketed expression may end up on its own line and we don't know which part exactly is meant to be silenced. I'm open to a pull request to improve this but my gut feeling is that it would be pretty tricky to get right. I hope that making the user adapt the reformatted code once so that other linters are happy isn't too much to ask.

What @zsol is suggesting will work, but you can also say what you suggested and Black will keep it:

from typing import (  # noqa
    Dict,
    List,
    Sequence,
    ...
)

[shish]

Yeah, I can see why this would be a very tricky special case - in my own code I manually moved the comment once, and black now keeps it in the correct place; it's just when I did the original one-off bulk-reformat of my project that a bunch of flake8 errors suddenly became un-silenced :)

[JelleZijlstra]

I ran into a similar issue with # type: ignore comments; black moved around my # type: ignores on some long lines, and now mypy no longer likes my code.

[ambv]

@JelleZijlstra, were you able to move # type: ignore back where you wanted it?

[shish]

I wonder if "when reformatting a one-line statement with a comment into a multi-line statement with a comment, put the comment at the end of the first line instead of at the end of the last line" would be a good approach? Running through some examples in my head I think that that should be equally consistent as the current behaviour, but it would Just Work more in more cases (and it feels more intuitive IMO - if I'm commenting a block of code, I put the comment at the start of the block)

[ambv]

I think what I can do for # noqa and # type: ignore is to move the comment after the opening parenthesis instead of after the closing parenthesis (because the latter is guaranteed to have no effect).

[zsol]

Maybe consider handling # pylint: comments as well, as described in #386

[nedbat]

This also affects coverage.py # pragma: no cover comments.

[Utsav2]

IMO, this can very easily devolve into "my tool requires these comments to be untouched, and black messes them up, can we special case them?"

On the other hand, practically every company needs to special case something. At my workplace, it's the type ignore comments.

Why not let us specify in a config the kind of comments we want to special case? That way it's easy for organizations to onboard onto Black without needing a fork/request new functionality.

I'm not sure how it would be implemented, so it's just a thought.

[cpennington]

I mentioned this over in #379, but I'll add it again here: I think a policy of leaving comments pinned to the start of the line they started on, rather than the end of the line they started on, would fix many/most of these problems.

[andersk]

I think a policy of leaving comments pinned to the start of the line they started on, rather than the end of the line they started on, would fix many/most of these problems.

That wouldn’t be enough for mypy # type: ignore, which applies to just the line it’s on, not the whole statement if it’s part of a longer statement.

value = (
    checked()
    + ignored() + ignored()  # type: ignore
    + checked()
)

Here’s a proposal that doesn’t require understanding whether each magic comment applies to the entire statement or some specific part. Could Black guarantee to apply only horizontal reformatting to lines with a comment matching a configurable set of prefixes (noqa, pragma:, pylint: type:)?

In other words, Black would disable the length limit for that line to avoid breaking it into multiple lines, and if the line is part of a longer statement, Black would preserve the line breaks preceding and succeeding the line. So it would not reformat the above to

value = (
    checked()
    + ignored()
    + ignored()  # type: ignore
    + checked()
)

nor to

value = checked() + ignored() + ignored() + checked()  # type: ignore

either of which would change the comment’s semantics.

[nedbat]

Could Black guarantee to apply only horizontal reformatting to lines with a comment matching a configurable set of prefixes (noqa, pragma:, pylint: type:)?

Why only apply this for certain prefixes? If a multi-line statement has a comment in a middle line, surely the author meant for it to apply to that line for a reason?

[cpennington]

Perhaps the easier rule is to just not have black expand or collapse any lines with trailing comments?

[andersk]

I think we still want Black to expand an overlong single-line statement with a normal comment at the end, even though that transformation is unsound for # type: ignore, hence the need for some kind of matching. Giving that up would be simpler but perhaps too simple.

Of course, if the horizontal-only behavior is configurable with prefixes, then you can configure the empty prefix to get it on all comments.

(I should mention where I'm coming from: I'm a big fan of Black, but this issue is a blocker for considering its use in Zulip, which was one of the early adopters of mypy and has hundreds of # type: ignore comments.)

[avivajpeyi]

I am also having issues with black re-formatting my imports that I want ignored:

import os
from typing import Optional

import matplotlib # noqa isort:skip fmt: off
matplotlib.use("agg")  # noqa isort:skip fmt: off
from matplotlib import pyplot as plt
from gwpy.signal import filter_design
from gwpy.timeseries import TimeSeries

After blacking:

import os
from typing import Optional

import matplotlib  # noqa isort:skip fmt: off

matplotlib.use("agg")  # noqa isort:skip fmt: off
from matplotlib import pyplot as plt
from gwpy.signal import filter_design
from gwpy.timeseries import TimeSeries

[hugovk]

@avivajpeyi I think that is unrelated and is expected behaviour. fmt: off must be used to mark the start of a block to ignore (with fmt: on at the end), not for a single line.

See https://github.com/psf/black/blob/master/README.md#the-black-code-style

There may be an existing issue for ignoring single lines, or please open a new design issue.

[hugovk]

Here's another example, repro with master and 19.10b0:

Black 243230

Playground link

Options

--line-length=88

Input

args = (
    (3, 3),
    # fmt: off
    (-1, -1,  0,
     -1,  0,  1,
      0,  1,  1),  # noqa: E127
    # fmt: on
    0.3,
)

Output

args = (
    (3, 3),
    # fmt: off
    (-1, -1,  0,
     -1,  0,  1,
      0,  1,  1),
    # noqa: E127
    # fmt: on
    0.3,
)

Expected

No change from input.

It's also a little surprising Black moves comments inside fmt: off/fmt: on.

---

This is more of a problem with the new Flake8 3.8. In Flake8 3.7, you could mark the whole block as noqa, and Black was fine with this (Playground link):

args = (  # noqa: E127
    (3, 3),
    # fmt: off
    (-1, -1,  0,
     -1,  0,  1,
      0,  1,  1),
    # fmt: on
    0.3,
)

But in Flake8 3.8 you need to mark the specific line and Black moves the comment rendering it ineffective.

[jaraco]

This issue affected me also, where I started with:

filterfalse = getattr(itertools, 'filterfalse', None) or itertools.ifilterfalse

which failed mypy due to the lack of ifilterfalse on some Pythons, so I suppressed the error:

filterfalse = getattr(itertools, 'filterfalse', None) or itertools.ifilterfalse  # type: ignore

but that failed flake8 checks due to long line, so I replaced it with:

filterfalse = getattr(itertools, 'filterfalse', None) \
    or itertools.ifilterfalse  # type: ignore

Which failed black, so I ran black, which generated:

filterfalse = (
    getattr(itertools, 'filterfalse', None) or itertools.ifilterfalse
)  # type: ignore

Which failed mypy because the ignore was on the wrong line.

😭

[andersk]

@jaraco Good example, although Black will preserve

filterfalse = (
    getattr(itertools, "filterfalse", None) or itertools.ifilterfalse  # type: ignore
)

(Might I suggest from six.moves import filterfalse? Or if it’s not too optimistic to assert that Python 2 is finally dead for real now, from itertools import filterfalse?)

[adigitoleo]

I hope it's not overkill to add another example here, but I just ran into this as well I think. Before:

from obspy.clients.fdsn.header import (  # type: ignore
    FDSNNoDataException as _FDSNNoDataException,
)

After:

from obspy.clients.fdsn.header import (
    FDSNNoDataException as _FDSNNoDataException,
)  # type: ignore

It's in parentheses because the line was more than 88 chars. In general this would be a problem for any very long import lines that need a # type: ignore. Also tried wrapping in #fmt: off/# fmt: on but the mypy comment gets moved.

@andersk Your idea of configurable ignore rules gets my vote, and to combine with what the other comments suggest maybe some of the most common things like # type:, # noqa: and # pylint: could be ingored 'by default'?

[JelleZijlstra]

We now have some code (in comments.py) that attempts to keep magical comments like # noqa in the right place, and #2272 added discussion of this topic to the docs.

[jonasrauber]

Shouldn't this work for these two cases?

def foo(parameter1: str, parameter2: str) -> None:  # pylint: disable=missing-return-doc,missing-function-docstring
    pass

def foo(parameter1: str, parameter2: str, parameter3: str, parameter4: str, parameter5: str, parameter6) -> None:  # noqa
    pass

After running black 22.3.0, the comments are still in the wrong place:

def foo(
    parameter1: str, parameter2: str
) -> None:  # pylint: disable=missing-return-doc,missing-function-docstring
    pass


def foo(
    parameter1: str,
    parameter2: str,
    parameter3: str,
    parameter4: str,
    parameter5: str,
    parameter6,
) -> None:  # noqa
    pass