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

Support # noqa: ... pragmas #3221

Closed
wants to merge 1 commit into from
Closed

Conversation

AWhetter
Copy link
Contributor

Steps

  • [*] Add yourself to CONTRIBUTORS if you are a new contributor.
  • [*] Add a ChangeLog entry describing what your PR does.
  • [*] If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • [*] Write a good description on what the PR does.

Description

Added support for understanding # noqa: ... pragmas. Bare # noqa pragmas are not supported.

Cc @sigmavirus24 who may be interested in this change. Also we've added support for parsing symbolic message IDs (eg no-member instead of E1101) but this would mean that flake8 cannot parse other messages in that same pragma (See doc/user_guide/message-control.rst). I don't know if you think it's worth making a change in flake8 to parse and ignore those types of IDs before we merge this?
There was also discussion on the original issue about whether we should be namespacing message IDs in pragmas to avoid conflicts.

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

Closes #2493

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 89.855% when pulling 7c24b62 on AWhetter:fix_2493 into 97f4f2a on PyCQA:master.

@coveralls
Copy link

coveralls commented Oct 29, 2019

Coverage Status

Coverage increased (+0.02%) to 89.86% when pulling 7c24b62 on AWhetter:fix_2493 into 97f4f2a on PyCQA:master.

Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is very cool, thanks @AWhetter ! We should check if it doesn't conflict with @hippo91 's pragma refactoring in #3076

@sigmavirus24
Copy link

Also, would it be worth splitting out the NoQA bits into a library that both Flake8 and PyLint can share so that we're not at odds with each other at any point?

cc @asottile

@asottile
Copy link
Contributor

Also, would it be worth splitting out the NoQA bits into a library that both Flake8 and PyLint can share so that we're not at odds with each other at any point?

cc @asottile

this patch is already problematic afaict -- # noqa: no-member, E501 is going to break flake8's parsing :S -- doesn't pylint already have # pylint: disable=... and not need noqa 🤔? (sorry if this is already addressed didn't read the issue just the code)

@asottile
Copy link
Contributor

it would also all-but-ruin my plan of making noqa of unknown codes an error in flake8

@PCManticore
Copy link
Contributor

@asottile Yeah, we already use our own pragma but the idea for this enhancement started from the fact that all the static analysis tools use their own pragma structure. We'd like folks to be able to use # noqa so they can migrate easily to pylint or from pylint so they won't have to rewrite their pragmas all the time. Of course, the messages will be different, but doing # noqa should be enough to silent pylint, flake8, pyflakes and whatnot.

I also agree with @sigmavirus24 that it would be great to split the common bits in a library if we decide to go this route.

@asottile
Copy link
Contributor

yeah I'd rather pylint not use flake8's noqa as it will break existing tools and make it impossible to implement the planned feature strict noqa

maybe a new pragma that we can all share? though it'll have the same drawbacks and (imo) not really much advantages (since you can't get bad-option-value from it for instance)

@AWhetter
Copy link
Contributor Author

AWhetter commented Nov 3, 2019

I'm on board with having a common library as well.
Sharing this pragma does mean that it's important for error codes between linters to never overlap (hence the suggestion of allowing namespaces). If someone does # noqa: E1111, E1111 needs to exist in only one of the linters. (Or it needs to represent the same error in both linters but that doesn't seem practical because many linters might have different ideas about the circumstances under which a message is raised).
I don't think we can enforce this easily because error codes can come from plugins. Even if the linters could advertise to the library what error codes they have, should they load all their plugins? What if some plugins have been disabled in config files or someone always disables a message on the command line? Loading all plugins seems like a lot of overhead. So the namespaces idea does seem essential. We should make them optional though. So you can do # noqa: E1111 or # noqa: pylint:E111.

The strict noqa problem suffers the same issue. How can we know all the valid error codes? I don't think it's possible.

@sigmavirus24
Copy link

So I think this becomes really annoying if we have to specify things: # noqa: flake8:E111,flake8:E231,pylint:E111 I don't want to special case things either.

@PCManticore
Copy link
Contributor

The consensus seem to be that at this point noqa is flake8 specific, and adding support for it in pylint would break existing tools. Having a namespaced noqa does not seem an improvement to what we already have today with # pylint, # noqa and # type: ... from mypy.
If that is the case, I'm fine with dropping this and continue with our own specific pragma for the time being.

@AWhetter
Copy link
Contributor Author

I agree with this as well.

@AWhetter AWhetter closed this Nov 16, 2019
@socketpair
Copy link

socketpair commented Nov 25, 2019

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

@asottile
Copy link
Contributor

I assume this works:

# noqa: ... # pylint:disable=...

@alexchandel
Copy link

noqa is not flake8-specific. It's used by several other linters, including pydocstyle. Please merge this.

@sigmavirus24
Copy link

noqa is not flake8-specific. It's used by several other linters, including pydocstyle. Please merge this.

pydocstyle, pycodestyle, and other tools that support noqa added support when they were already using violation identifiers that worked with Flake8 and already plugged into Flake8. pylint's older, has it's own identifiers and those identifiers are incompatible with Flake8's, pydocstyle's, and pycodestyle's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for noqa: ERROR MESSAGE
7 participants