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

# noqa not honored for most Errors #180

Closed
stchris opened this issue Mar 18, 2013 · 10 comments
Closed

# noqa not honored for most Errors #180

stchris opened this issue Mar 18, 2013 · 10 comments
Labels

Comments

@stchris
Copy link

stchris commented Mar 18, 2013

Quoting from https://bitbucket.org/tarek/flake8/issue/37/pep8-doesnt-honor-noqa: " Only the E501 (line length) and the E12* (continuation line) benefit from this feature.", while "# noqa" should work on all kinds of errors and warnings.

Example:

a = 1
if a == None:  # noqa
  pass
@florentx
Copy link
Contributor

I confirm that # noqa is only active for E501 and E12* errors.
It should be documented as such.

The best way to disable a checker is through a tox.ini or a setup.cfg file.
We don't want to contaminate all Python files with such noise. It's not the purpose of these tools.
This is why it's probably not a good thing to disable all the checkers when the user passes # noqa, it will encourage such misuse. (I hate these # pylint: or # noqa comments which are useless for the reader)

I guess that your use case is about SQLAlchemy, no?
If it makes sense, we might consider adding the E711/E712 errors to the coverage of # noqa.

@stchris
Copy link
Author

stchris commented Mar 19, 2013

Yes, in my case it is about SQLAlchemy == comparison magic and I'd
appreciate an exception to this rule. But more generally, I'd expect noqa
to work as documented, leaving it to the developer to decide when/if to use
it. I fully agree that it shouldn't be used excessively.

I confirm that # noqa is only active for E501 and E12* errors.
It should be documented as such.

The best way to disable a checker is through a tox.ini or a setup.cfgfile.
We don't want to contaminate all Python files with such noise. It's not
the purpose of these tools.
This is why it's probably not a good thing to disable all the checkers
when the user passes # noqa, it will encourage such misuse. (I hate these #
pylint: or # noqa comments which are useless for the reader)

I guess that your use case is about SQLAlchemy, no?
If it makes sense, we might consider adding the E711/E712 errors to the
coverage of # noqa.


Reply to this email directly or view it on GitHubhttps://github.com//issues/180#issuecomment-15137607
.

@sigmavirus24
Copy link
Member

Personally, I'm entirely for giving users all the freedom they might like to do whatever they please to their code. I agree it doesn't add anything useful, but it is their code and their choice.

@glarrain
Copy link

@sigmavirus24 Isn't PEP8 all about NOT giving the developer the choice to do whatever they please to their code? The purpose of conventions or standards is to help different people/organizations understand each others work, without having to wonder what might the author mean with this or that.

@sigmavirus24
Copy link
Member

@glarrain PEP8 is about giving the developer a set of guidelines that they may or may not follow. It even says, that there are cases where you may want to diverge from it and when they make sense that's fine. The difference is that this tool checks those for you, so in 99% of the cases people probably won't fill their files with # noqa and if they do, the only question in your head should be "Why do they even use pep8 at all?" In short, if they don't want to observe a rule on a single line, for whatever reason, then let them ignore it. They're shooting themselves in the foot in my opinion. This may hurt the next person, but we can't look out for everybody and if the person authoring the code doesn't care about them, there isn't much we will be able to do to change that. The developer can also, simply do: pep8 --ignore=Exx ... and then not only are they ignoring that line, they're ignoring every one of those errors in every file. Which is less annoying? One file with one item ignored or an entire project with one error ignored because we didn't allow one line's error to be ignored?

@florentx
Copy link
Contributor

Which is less annoying? One file with one item ignored or an entire project with one error
ignored because we didn't allow one line's error to be ignored?

Globally I agree with people which think that the pep8 tool should not encourage to add comments like # noqa at line level. This is counter-productive because it adds noise to the comments and it does not help writing better code.

There's some options if some error/warning seems inaccurate:

  • accept the tool is not perfect, and use your own judgement
  • report the issue so it can be fixed in the tool, for everyone
  • disable this specific check in tox.ini or setup.cfg

The issue with # noqa is that it will end like # pylint: and contaminate all your codebase.
It might look easier to add # noqa everywhere instead of discussing the point with the other developers of the team: some particular rule might be not relevant for a project. And people don't even know that configuration is allowed per project.
The PEP-8 says it: "Consistency within a project is more important."

And if the issue is genuine, the # noqa will remain for a long time, even if a new version of the tool fixes it.
We already have such example with Pyflakes. There were lots of "Redefined variables" because of compatibility code like this:

try:
    import json
except ImportError:
    import simplejson as json    # NOQA

It was fixed with latest Pyflakes, but the comments # noqa will remain for a long time where it is used.

For the case reported here (a == None), maybe we will add the E711/E712 errors to the coverage of # noqa.
But for my own project, if I was using SQLAlchemy, I would prefer to disable these checks in the configuration for the whole project. Sparingly, I will run the checker for E711/E712 manually on the code, to verify there's no real case which needs fixing.

nidico added a commit to liqd/adhocracy that referenced this issue Jul 8, 2013
Pep8 now honors the `# noqa` comment, which disables pep8 checks for a
certain line (see PyCQA/pycodestyle#180).

This is useful for SQLalchemy `== None` filter comparisons and used all
quite a bit in Adhocracy model methods, so this should reduce our pep8
error counter quite a bit.
@cbeland
Copy link

cbeland commented Dec 11, 2013

I had this problem with "E221 multiple spaces before operator" because I wanted to:

var_short          = 0.0010
var_long_name      = 0.00010
var_very_long_name = 0.0100

and it is far more important to have the = signs line up so the decimal places are clear, than it is to comply with the whitespace standard.

In my opinion, it would be nice if # noqa worked for any warning, since a clean compliance report is mandatory for us, and turning off compliance for the entire file is the only alternative (which is much worse) in any situation we feel we need # noqa to improve readability. I agree it can be abused, but that is the only situation in which I would use it, and it seems like by using it as intended I should get the best possible result.

@jayvdb
Copy link
Member

jayvdb commented Nov 29, 2015

For indirect use of pep8 via flake8, there is an extension which provides this (by fiddling with the default flake8 reporter, so it only works with jobs = 1):
https://pypi.python.org/pypi/flake8-respect-noqa

I have built a flake8 extension which provides per-file and per-line configuration out of the source tree, by fiddling with the ignore_code part of the error reporting (and thus even more hacky, but works with jobs > 1): https://github.com/jayvdb/flake8-putty

@scott-ainsworth
Copy link

I want to reinforce @cbeland's point. There are many cases where columnar alignment provides more clarity than strict compliance with whitespace rules.

Reading the comments above, the primary argument against broad application of #noqa is the addition of noise in the source code—a real concern. Another real concern is overuse of global suppression (e.g., via setup.cfg). Many programmers use Eclipse and other IDEs configured to show PEP 8 warnings all the time. (After all, keeping code PEP 8 compliant is usually easier than fixing it later.) However, if global suppression is avoided and local suppression not available, the noise created within the IDE becomes a problem.

This situation is compounded on team projects. The only practical team approach to PEP 8 warnings is elimination of all warnings. Without local suppression, global suppression is the only option—and global suppression leads to missed problems.

Please enable suppressing all rules, or at least the whitespace rules (and E221 in particular),with #noqa. PEP 8 is the standard for the Python Standard Library. For all other products, let's leave suppression decisions to the product team.

@IanLee1521
Copy link
Member

Closing, see call for pull requests in #480.

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

No branches or pull requests

8 participants