Make pylint consider disable params when checking for line length

[DanielNoord]

Current problem

As discussed in #1682 pylint currently does not consider the length of the disable params when checking for line length.
For example, when max-length is set to 80 the following code would not raise a line-too-long warning.

Line with length of 80 columns  # pylint: disable=not-callable

According to the issue, this is due to:
https://github.com/PyCQA/pylint/blob/14da5ce258d3818037304a6e0f61aba1462e251d/pylint/checkers/format.py#L1004
Which seems fair.

However, this is not what users might intuitively expect from this warning, as a comment in that issue shows. Some users might want the max-length to also include comments.
Although #4797 fixes some of the issues in the original issue, it does not solve this particular issue. This issue serves to track it.

Desired solution

Make pylint consider comments when checking for max-length of lines.

Open question:
What should be default behaviour? Including comments or excluding comments? Current behaviour is excluding, but I would argue that it would be more intuitive to include them. Especially now that users can use disable-next to split comments and code over multiple lines.

[Pierre-Sassoulas]

Wow, I did not understand that the current behavior was to exclude the length of the comment. You're right to have insisted ! Clearly in my mind the default should be to take the real length of the line into account. We could maybe suggest to put the comment on the line above if it's the comment that is causing the problem (and to use disable-next if it's a pylint comment). Might be an issue with other linter like flake8 that do not have # noqa-next ?

[DanielNoord]

Hmm perhaps, but for those cases users will already have disabled line-too-long.
I think we can change the default behaviour and suggest to start using disable-next. Perhaps another maintainer wants to have a say about this?

[Pierre-Sassoulas]

The issue I was seeing is if pylint is suggesting to move a noqa from flake8 to the line above when it's in fact impossible. Maybe we don't need to handle that use case.

@cdce8p do you have an opinion ?

[cdce8p]

There seem to be valid arguments for both sides, with or without comments. Tbh I don't really like either that much.

Should we still have this check when flake does it probably way better than we do? Might be worth removing checks (or moving them to an extension) that are redundant (with flake) as we wouldn't need to support them anymore. If I remember correctly there have been some discussions here #3512 but I didn't follow that closely so don't know what the consensus was.

[Pierre-Sassoulas]

Might be worth removing checks (or moving them to an extension) that are redundant (with flake) as we wouldn't need to support them anymore.

We only document the one that are redundant if you're already using another linter, because some users want to just use pylint exclusively. Because of that removing checkers from the default is only decided on the checker's own merit regardless of another linter doing it or not. With the exception of very hard to maintain checks (like bad-indentation when black can do the formatting). line-too-long is not hard to maintain I think (more work than I thought though 😄 ).

For this issue I suggest we take the real line length into account for line-too-long and add . Use '# pylint: disable-next=x' on the previous line. to the current reported message if the line contain pylint: disable=x in it.

[yilei]

I would suggest keeping the default excluding the comments. Even though there are now disable-next, this would put extra burden to the users to change the trailing # pylint: disable= to a new disable-next in a separate line above.

This happens both to existing code, and when you write new code interactively. When you add a trailing # pylint: disable=, you don't know in advance whether the newly added disable= will fit in the line length or not. So you need to try it out, and if it doesn't fit, you have to rewrite it to disable-next on a different line.

Also, I've collected some stats over our internal code base regarding how often our trailing # pylint: disable= exceeds the line length. I sample 2000 files that contain trailing # pylint: disable lines. Among these files:

1. Total number of lines with trailing # pylint: disable=: 4089
2. Number of lines that's within column limit: 2401
3. Number of lines excluding the pragma that's within column limit: 1216

So that happens a lot to existing code base that's used to the current Pylint behavior.

[DanielNoord]

@yilei Thanks for the thorough investigation! Based on this I think it makes sense to keep excluding comments from line length. This seems like a major breaking change and if I had to change +-1200 lines just to be able to upgrade to pylint 3.x I probably wouldn't.

Although I think it makes sense to do this, with the current status quo I'm -1 on this proposal.

[Pierre-Sassoulas]

Thanks you for the feedback @yilei, I did not realize the change would create that much work. Clearly we can't do it unless we automate the move to disable-next one way or another. (Off topic: An auto-fixing pylint is talked about often but some design is required, and strangely, no issue is opened for it. Imo if we start somewhere and add things progressively it could work. While making some messages better you feel that you can give the exact code replacement so why not automatically replace the code instead).

Should we close @DanielNoord ?

[DanielNoord]

I think an autopylint like autoflake8 should probably be its own project. It will need its own release schedule and shouldn't be impacted by pylint development.

I think so... Although we might want to document this somewhere a little more explicitly?

[yilei]

+1 for auto-fixer to be its own project to separate concerns. pylint itself can be the auto-fixer's upstream dependency. In fact, we do have something like that internally to auto fix a few of the linter errors. For cases like bad-whitespace, multiple-statements, it invokes a formatter to achieve this. So it also has a lot more dependencies than pylint. It also needs its own interface when integrating with editors/git.

[Pierre-Sassoulas]

I'll open an issue for autofixing.

Let's close this issue with a documentation merge request then ?