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

Add support for noqa: ERROR MESSAGE #2493

Open
PCManticore opened this issue Sep 15, 2018 · 19 comments
Open

Add support for noqa: ERROR MESSAGE #2493

PCManticore opened this issue Sep 15, 2018 · 19 comments
Labels
Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc.
Milestone

Comments

@PCManticore
Copy link
Contributor

Is your feature request related to a problem? Please describe

Yes. The problem is that every linter out there has its own pragma control, leading to a mess if you want to support bandit, pylint, pyflakes, flake8, mypy:
… # type: … # noqa: E501 # pylint: disable=line-too-long # nosec

I think it would help if we'd also support noqa with an error message (not bare noqa which I'm still against using in pylint). It could also help with the adoption as well with keeping your codebase linter agnostic if you plan to use just pylint and flake8.

Describe the solution you'd like

Support noqa: error message so noqa: no-member or noqa: E1101 in pylint.

@PCManticore PCManticore added the Enhancement ✨ Improvement to a component label Sep 15, 2018
@tuukkamustonen
Copy link

tuukkamustonen commented Oct 11, 2018

Would it need some extra identifier in noqa: E1101 to indicate that we are about to ignore a pylint error and not error from some other tool? Is there really no overlap between pylint, pycodestyle, pydocstyle, flake8, others?

Also, would this work only per-line or also per-file/block (like # pylint: disable)?

@PCManticore
Copy link
Contributor Author

@tuukkamustonen We probably won't need to indicate that we're ignoring a pylint or say a flake8 error. We can recognise our errors and hopefully other tools don't have an overlap with ours but with a different meaning (e.g. E1101 to have a separate meaning in pylint vs flake8) The plan here is to help onboard folks on using pylint, if they don't have to change the pragmas just for its sake.

Regarding the scoping of the pragma, it will work as the current # pylint pragma, the new noqa will be just an alias of # pylint

@The-Compiler
Copy link
Contributor

FWIW I love this idea simply because # noqa: no-member is much shorter than # pylint: disable=no-member, so there are higher chances of easily fitting it after a line of code with a conservative line width limit.

@edran
Copy link

edran commented Oct 23, 2018

Could you not add a namespace to differentiate between flake8 and pylint errors?

For instance instead of parsing the error codes directly, you could match codes that look like pylint-errorcode, so that you could then mix the errors at will.

I.e.:

# noqa: flake8error, pylint-error

@PCManticore
Copy link
Contributor Author

PCManticore commented Oct 26, 2018

@edran I don't understand what you mean. Are you referring to add a pylint- or flake8- prefix to the error codes? That unfortunately doesn't seem like a big of change than the current situation where you'd use # pylint instead and it would probably be a larger discussion for the whole PyCQA org to find a common ground on the noqa support. What this current issue tries to accomplish is to allow folks that already use noqa to switch to pylint without too much hassle.

@lokesh1729
Copy link

👍+1

AWhetter added a commit to AWhetter/pylint that referenced this issue Oct 29, 2019
AWhetter added a commit to AWhetter/pylint that referenced this issue Oct 29, 2019
@AWhetter
Copy link
Contributor

Upon further discussion, is does not seem possible for pylint to introduce #noqa support without breaking things in flake8, and making maintenance more difficult in the future for both projects. Therefore I'm closing this as something that we are unable to implement.

@socketpair
Copy link

So, just how to use noqa and pylint: disable=xxxxx on the same line ?!

@AWhetter AWhetter mentioned this issue Jan 30, 2020
@rachmadaniHaryono
Copy link

So, just how to use noqa and pylint: disable=xxxxx on the same line ?!

#2470 (comment)

You can rewrite your comment something along the lines of # pylint: disable=; # noqa, this should make it work.

in my case

temp = 'long-text'  # pylint: disable=line-too-long # noqa: E501

@hippo91
Copy link
Contributor

hippo91 commented Oct 29, 2020

@rachmadaniHaryono thanks for this hint!

@EricDuminil
Copy link

@rachmadaniHaryono : Sooo... if a line is too long, your advice is to append 46 chars of unreadable boilerplate comments, which have to be written just the right way?

@Pierre-Sassoulas
Copy link
Member

Current best solution is to do:

# pylint: disable-next=line-too-long 
temp = 'long-text'  # noqa: E501

noqa is not specific to flake8, in particular since ruff uses it too. The discussion that went on in #3221 is now outdated. Reopening for discussions / specifications.

I think that this would be confusing:

temp = 'long-text'  # noqa: E501,line-too-long 

But pylint not raising in this case would make sense:

temp = 'long-text'  # noqa

Maybe even in this case:

temp = 'long-text'  # noqa: E501

@Pierre-Sassoulas Pierre-Sassoulas added Needs decision 🔒 Needs a decision before implemention or rejection Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. labels Mar 22, 2023
@ml31415
Copy link

ml31415 commented Apr 13, 2023

Is it really that hard, to respect all the # noqa syntax, just as if it were original pylint comment instructions? For compatibility reasons, maybe behind a command line flag? I really don't think so.

It's just super annoying if e.g. your IDE wants one style of marking issues, and pylint another one.

@The-Compiler
Copy link
Contributor

@ml31415 There is no clear 1:1 mapping from flake8 IDs to pylint messages. Someone would need to create a mapping between all the ones considered equivalent (and that needs to be kept up to date).

Things are always "not that hard" when you're not the person actually doing the work 😉

@ml31415
Copy link

ml31415 commented Apr 14, 2023

@The-Compiler it doesn't need to be perfect. Even ignoring any message for the line, when there is # noqa in there would already be helpful.

@Pierre-Sassoulas
Copy link
Member

I re-read the description of the issues and I understand why PCManticore was against raw # noqa. Right now pylint selectively disable comments, it's impossible to just disable everything on a line (so you don't miss a message disabled by mistake). There's also performance implication of doing twice the comment parsing. On the other hand a lot of tool permits to do noqa and this is an easy/fast/instinctive way to disable a message for a lot of person. Let's put this to a vote:

  • Allow # noqa disable for disabling everything in pylint: ❤️
  • Allow # noqa no-member or # noqa E1101 disable for disabling "no-member" in pylint: 👀
  • Keep only supporting # pylint: ...type comment: 🎉

Maintaining an (up to date) list of equivalent message in other tool is another issue, I've opened #8579 to follow-up.

@ml31415
Copy link

ml31415 commented Apr 15, 2023

@Pierre-Sassoulas option 1 doesn't need to be default behaviour. It would totally be sufficient, if this behaviour is hidden behind a command line flag. This poll is well intended, but such a command line flag --use-noqa or whatever it might be called, would just give everyone the chance to have it his way.

@Pierre-Sassoulas
Copy link
Member

Yeah my bad, you're right that this survey is useless, everyone reading is interested by this issue and is going to vote to fix it. Also the interest in noqa handling is obvious seeing there's 20+ upvote and 24 angry down-votes when we said we weren't going to do it. Also if we can make # noqa no-member or # noqa E1101 works at a small cost we should do it and the decision is for maintainers to take.

I like the idea of adding an option for noqa. We can consider a # noqa: E1101 comment equivalent to pylint: disable=E1101 and # noqa to pylint: disable=all and change very little code to make it work. The problem is that a part of the "very little code" we need to change is a 10 lines regex that was optimized over multiple merge requests to prevent catastrophic backtracking and other non trivial regex's surprise (#5925 / #5786 being the latest installment):

OPTION_RGX = r"""
(?:^\s*\#.*|\s*| # Comment line, or whitespaces,
\s*\#.*(?=\#.*?\bpylint:)) # or a beginning of an inline comment
# followed by "pylint:" pragma
(\# # Beginning of comment
.*? # Anything (as little as possible)
\bpylint: # pylint word and column
\s* # Any number of whitespaces
([^;#\n]+)) # Anything except semicolon or hash or
# newline (it is the second matched group)
# and end of the first matched group
[;#]{0,1} # From 0 to 1 repetition of semicolon or hash

We could start to be stricter in what we consider a good disable, for example by being exactly as strict as flake8 / ruff in order to simplify this. It's breaking change season after all. It might also be a good time to remove the very old # pylint: disable-msg= / # pylint: enable-msg= or at least deprecate it.

("disable-next", "disable-msg", "enable-msg", "disable", "enable")

@Flamefire
Copy link

I would also like support for # noqa to just disable any diagnostic of that line. Yes it might be to broad but use of this is up to the user/author of the code. Maybe there could even be a pylint diagnostic (disabled by default) to flag plain # noqa, i.e. without any other text behind such that one could do # noqa - used later and not have that flagged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.