pylint inline comments wrapped to wrong line

[drootang]

Pylint inline comments on wrong line after wrapping long line.

Inline comments that trail a command continue to trail the command after black wraps a long line onto multiple lines. While seemingly consistent with the original form, this makes inline comments like #pylint: disable=invalid-name ineffective since they are not on the first line of the command. For example:

asdf = namedtuple('nameoftuplegoeshere', 'fieldsgohere andnoather andanother morehere') # pylint: disadble=invalid-name

becomes

asdf = namedtuple(
    "nameoftuplegoeshere", "fieldsgohere andnoather andanother morehere"
)  # pylint: disadble=invalid-name

For pylint to work properly, it needs to be:

asdf = namedtuple( # pylint: disadble=invalid-name
    "nameoftuplegoeshere", "fieldsgohere andnoather andanother morehere"
)

Since the #pylint comment is no longer on the same line as asdf = it does not suppress the pylint warning, changing the behavior of the comment.

While not technically a bug, and not changing the functionality of the code, it does have an undesirable effect. Is there some philosophy I'm missing, or is there a recommendation for how to handle this?

[felix-hilden]

Thanks for submitting! We'll have to consider if there's a reasonable way for us to recognise where the comment should go. But failing that, I'd advise to move the comment by hand. Unsatisfying, I know, and annoying if the line is later changed and collapsed back. Dealing with meaningful comments is pretty hard 😄 particularly when splitting lines.

---

If you have any ideas, please share. For the time being, I can't think of anything other than exactly knowing what each comment means and trying to target the correct line. But I'm guessing invalid-name could refer inside the call as well:

a = (
    inValidName := value  # pylint: disable=invalid-name
)

And if this is the case, I'd be inclined to not even try to do anything 😅

[JelleZijlstra]

Note that we already special case pylint comments: https://github.com/psf/black/blob/main/src/black/comments.py#L266. Perhaps that can be improved, but as @felix-hilden says, it's very hard to make this perfect because we don't know what the comment refers to.

[unitysipu]

just an alternative, make it configurable to prevent formatting on lines with pylint definitions on them?

[yilei]

One improvement to the handling of those special cased pragma comments I can see is, when the comment is after a closing paren and when it reformats the statement to put the closing paren on its own line, move the comment to after the opening paren instead of the closing paren.

This solves the case for comment #1, not comment #2. But this is still going to be a good improvement to make. AFAIK, the comment after the closing paren on its own line will never have any effect (true for # noqa, # typing: ignore, pylint: disable= etc..).

Bonus: also include closing paren + comma (),) on their own line:

check_call(
    program,
    lambda: lib._some_private_module(param1, param2, to_make_it_long), # pylint: disable=protected-access
    arg2)

check_call(
    program,
    lambda: lib._some_private_module(  # pylint: disable=protected-access
        param1, param2, to_make_it_long
    ),  
    arg2,
)

[jaraco]

This issue has affected me on multiple occasions when invoking black on functions that use noqa: C901 (example). Interestingly, the issue seems not to manifest on later Pythons, so the noqa after the closing parenthesis has the intended effect on Python 3.8+ but not on 3.7. And since I can't even get Python 3.7 on my system (Apple Silicon macOS), I don't see the error until it hits CI.

[jaraco]

Oh, and it looks like the issue is even more complicated. This run indicates that putting the noqa comment after the open parenthesis isn't adequate in some cases. It turned out that I needed the noqa in two places (introducing a decorator revealed the disparity). That's probably a flake8 issue more than a black issue.

[yilei]

We are agreeing that Black can't know the perfect location of the pragma, but I do think we are able to statistically improve the heuristic here.

To get to a better heuristic, I collected some data from our internal code base for # pylint: disable= pragmas (since we use Pylint).

I have a local patch to Black to move the pragma to different places:

1. status-quo: The current Black's behavior
2. open-paren: Move it to the opening paren as described in pylint inline comments wrapped to wrong line #2843 (comment)
3. open-paren-allow-exceeding: Like open-paren, but do not wrap the line if it's only the # pylint: disable= pragma (including the leading spaces before #) that exceed the line length. This is actually Pylint's behavior (though the default might change in the future as described in Make pylint consider disable params when checking for line length pylint-dev/pylint#4802).

We use 80 column limit. Since the code base is mostly lint free, to simulate what happens when Black reformats the line that's too long, I did the formatting using a 60 column limit.

I collected 6333 files that has exactly one line that:

1. Has a trailing # pylint: disable=
2. Is longer than 60

After formatting, here are the number of cases that don't work:

1. status-quo: 2838 (44.8%)
2. open-paren: 1527 (24.1%)
3. open-paren-allow-exceeding: 931 (14.7%)

These numbers are of course biased towards our own code base and Pylint, and I don't have a good idea for # typing: and flake8 pragmas. And the open-paren-allow-exceeding behavior only works for Pylint comments (AFAIK not flake8).

That being said, I'd like to propose:

1. Make Black move the pragma to the opening paren
2. And ideally, for Pylint specific pragmas, also allow the pragma part to exceed the line length

Thoughts?

[DanielNoord]

@yilei open-paren seems to make sense to me. There are many cases where this won't work as many linting errors are raised on ast nodes somewhere in the middle of line, but it is definitely an improvement over putting them at the end of the line.

I was wondering though, if you are indeed going to special case pylint would you consider creating a # pylint: disable-next: comment on the line above the original line? This option was released with pylint-dev/pylint#4797 in pylint 2.10 on 2021-08-21. So over a year ago. According to PePy the largest part of our downloads are indeed 2.10+. I think in general, disabling with disable-next will cover more cases than placing on the opening parenthesis.
Feel free to disregard this if you don't want to include such a version dependent feature or if this is too much extra work to support an external project (I can also help with implementing this with some guidance on the codebase). I just thought I would present it as an alternative.

Thanks again for the thorough investigation and getting back to us in our own repo. Much appreciated!

@Pierre-Sassoulas Tagging you as you might be interested in this issue

[Pierre-Sassoulas]

I'm guessing this is not something that would be accepted by the black team (because this is so pylint specific) but automatically changing pylint: disable to pylint: disable-next on the previous line for line that are too long (but become the right length without the pylint comment...) could remove a lof of problematic cases too.

I.e. this:

asdf = namedtuple('nameoftuplegoeshere', 'fieldsgohere andnoather andanother morehere') # pylint: disadble=invalid-name

Could become this:

# pylint: disable-next=invalid-name
asdf = namedtuple("nameoftuplegoeshere", "fieldsgohere andnoather andanother morehere")

This does not solve the case where the line is still too long of course. But it could diminish greatly what the percentage of case that don't work really amount to in @yilei's data. The pylint comment is taking 20 characters without even specifying the message to disable, so I have the intuition that a decent percentage of lines would be of acceptable length without it. I could work on this if this is a desirable change.

[JelleZijlstra]

I would favor @yilei's open-paren option here.

[jaraco]

I encountered this issue again today in pypa/setuptools@8c21342. That diff was created by running a recent black. On line 859 (original line 786), there's a # noqa directive (employed flake8 originally and now for ruff to allow the long lines that appear in that string). The black reformatting moved the directive from the relevant string to the ).format( syntax, where it has no effect.