Add per file disabling of warnings

[IanLee1521]

Per "suggestion" of @dubglan in https://github.com/jcrocholl/pep8/issues/264#issuecomment-73584055, add the ability to disable warnings on a per file basis (ala pylint). Something like:

# pep8: ignore=EXXX

[sigmavirus24]

In the past @florentx has been strong :-1: on this. The issue tracker has to have at least one prior issue discussing this and the exact reasons it has been rejected as a feature repeatedly.

[dubglan]

@sigmavirus24 I recall some argument about "source noise".

The more checks are added to pep8, the more painful this issue becomes. #264 discusses a controversial check that imports must go in the beginning of the file. It almost always makes sense, but sometimes it doesn't, e.g. in case of conditional imports. The standard itself is not 100% clear on this. It doesn't say "never use conditional imports". And it's a common knowledge that conditional imports are widely used in practice.

The problem with pep8 is that there's no flexible mechanism for style exceptions: 1) # noqa doesn't work on the whole file, and 2) when it comes to other errors, I'd like to honor them, 3) adding # noqa to every single import line creates more noise than a single error-specific disable comment.

The fact that this feature request reappears despite being rejected speaks for itself.

[IanLee1521]

So I couldn't find an issue (quick search) rejecting this sort of a feature, but I did find some discussion in #180 that talks about the conflict between giving users the freedom to do what they want with their code (e.g. # noqa ala our discussion over on #304). Based on your comments at the time over their, you yourself were also in favor of more permissive use of # noqa and the like.

I completely agree that the use of that option should be discouraged and frowned upon (and by extension that code reviews should raise an eyebrow at the addition of such). However, the use or not use of such options should be the choice of the developer, not the tool.

Perhaps an additional thought is that there should be a check (likely disabled by default) that would report out when lines (or files if the feature proposed here is added) are excluded from the checks. That way there could be a second pass, in a CI system for instance, where you just look for that, and maybe you have a policy that allows only so many instances. E.g.:

# Assuming Exyz is "Lines should not contain ``# noqa``"
#! /bin/bash

$ violations=pep8 --ignore=E,W --select=Exyz | wc -l
$ [[ $violations -lt 10 ]] || echo "Too many # noqa lines"

[sigmavirus24]

The more checks are added to pep8, the more painful this issue becomes.

In other words, the more checks, the more things you want to ignore because you don't care about compliance but instead leaving things as they are?

264 discusses a controversial check that imports must go in the beginning of the file. It almost always makes sense, but sometimes it doesn't, e.g. in case of conditional imports. The standard itself is not 100% clear on this. It doesn't say "never use conditional imports". And it's a common knowledge that conditional imports are widely used in practice.

Conditional imports, as I recall, were in fact accounted for in #264, so I'm not sure why you're complaining about their necessity here. I also like how you think I don't know anything about how common conditional imports are.

1) # noqa doesn't work on the whole file, and

There's already functionality for that in tools that wrap pep8. pep8 doesn't need to have every feature every developer wants.

3) adding # noqa to every single import line creates more noise than a single error-specific disable comment.

Which is the reason # noqa should be removed from pep8 in my honest opinion. It was a mistake for us to add it in the first place.

The fact that this feature request reappears despite being rejected speaks for itself.

It speaks to the quality of the tool and the laziness of people who use it to pretend to be compliant with the style suggested by PEP-0008. I've yet to see a compelling reason for needing file-specific exclusions of rules. The only common one I've seen has been "but we vendor that code" which makes me wonder why people are checking it at all when they shouldn't be modifying it in the first place.

Based on your comments at the time over their, you yourself were also in favor of more permissive use of # noqa and the like.

I probably was. People do change though. The reality is that I've seen enough terrible code-review of contributions that ignore when someone adds # noqa for a check that the rest of the project doesn't ignore. People use this tool as a benchmark for the quality of contributions and that practice is becoming increasingly popular. By removing the necessity for reviewers to check for style violations we make their lives easier. At the same time, by adding more ways for contributors to add code that looks close enough but isn't quite right by disabling file-wide checks and adding ever more checks that can be silenced by # noqa we're adding more things for maintainers and reviewers to be aware of and not actually reducing their workload.

I completely agree that the use of that option should be discouraged and frowned upon

There's no way to indicate discouragement by adding a feature. Unless you plan to emit warning codes for use of this in a file that cannot be silenced. The only way to discourage its use is to make it inconvenient to use or to cause it to generate more feedback as a result. The idea of adding this feature and then saying "But don't do this" is preposterous.

However, the use or not use of such options should be the choice of the developer, not the tool.

I disagree with you yet again. pep8 is easy enough for someone who hasn't previously contributed to it to take over maintenance of precisely because its features are narrowly defined and its scope is well limited. Opinionated constraints make the tool more reliable and both contributors and maintainers lives easier. If users disagree with the opinions that inform tool creation they can fork it and force their own opinions on it. That's the definition of Open Source. It is not the definition of open source (especially not sustainable open source) to be everything to everyone.

That way there could be a second pass, in a CI system for instance, where you just look for that, and maybe you have a policy that allows only so many instances.

Anything that requires a second-pass or extra configuration is never going to be used. It would need to be something someone can add as a parameter to --select or it would need to just be on by default.

[dubglan]

Conditional imports, as I recall, were in fact accounted for in #264, so I'm not sure why you're complaining about their necessity here. I also like how you think I don't know anything about how common conditional imports are.

I was making an example of controversial check. Didn't want to offend you in any way or side-track the conversation.

There's already functionality for that in tools that wrap pep8. pep8 doesn't need to have every feature every developer wants.

Please make an example. We use tox which runs pep8 on the the source tree. Are you proposing we run it on per-file basis and filter out files we don't want to check? Certainly we can do that, but I'd prefer to track style exceptions together with code in exact same place where the exception is made. Do you agree that this makes sense and might be a valid proposal?

laziness of people

please don't make assumptions about other people, they are software engineers just like you and they have reasons, see below

I've yet to see a compelling reason for needing file-specific exclusions of rules.

Again, let me use conditional imports as an example. We rely on Pyro4 library which, unfortunately, requires doing something like

os.environ["PYRO_XXX"]="Y"
import Pyro4

or, let's say, you have to manipulate sys.path. Are these good practices? No! They do violate both the standard and other best practices. But sometimes you have to do that to simplify the process. For instance, without sys.path tricks you'd have to set up a full python packaging workflow to manipulate the path in a "legal" way. But proper python packaging requires special CI and internal mirrors (a la devpi).

[MarSik]

The same applies to mocking in test files. You often have to manipulate imports to be able to test your behaviour without importing something destructive in the nested (in terms of imports) modules.

[kleptog]

Sorry I'm late to the conversation, but I think the real problem is treating PEP8 as gospel for which no exceptions are possible. There are several situations where I feel PEP8 enforces bad looking code by disallowing certain idioms. Examples (mostly from SQLAlchemy because in my experience that is where it generates the most pain):

my_table = Table('mytable', metadata,
    Column('id', Integer, primary_key=True),
    Column('foo_id', Integer, ForeignKey('alerts.sensors.id'), nullable=False),
    Column('data', Text, nullable=False),

PEP8 rejects this because the hanging indents are not allowed with arguments on the first line. You can move the 'mytable' onto the next line, but IMHO that's less readable. I think the hanging indent rule should be relaxed in this case but for now I just want to forget about the hanging indent failure for this file (which contains only definitions like this), not for the entire project.

Or the following (first the readable version):

STATUS_NOT_YET_OPENED                 = 0
STATUS_OPEN                           = 1
STATUS_CLOSED                         = 2

PEP8 requires the following:

STATUS_NOT_YET_OPENED = 0
STATUS_OPEN = 1
STATUS_CLOSED = 2

Not what I call more readable. You should be allowed to relax the extraneous space rule in cases where it allows 3 or more lines to line up their equal signs. That would help in other cases as well.

foo = Bar(blah='gibberish',
          gobbledigook=2,
          wah=dict(foo='baz'))

vs

foo = Bar(blah         = 'gibberish',
          gobbledigook = 2,
          wah          = dict(foo='baz'))

The same for other operators would be nice as well (the following is not pep8):

return Session.query(Bar) \
              .filter(Bar.id == self.rul_id) \
              .filter(Bar.action != Bar.ACTION_DELETED) \
              .order_by(Bar.id.desc()) \
              .first()

PEP8 requires you to align the dots with some multiple of 4 bytes, which basically never ends up anywhere useful. Basically, it is not true that pep8 automatically leads to more readable code and since it isn't possible to make small tweaks we need sledgehammers like noqa and filtering in pyflakes.

I feel pep8 spends so much time being strict on the little things that it sometimes misses the bigger picture. It'd be awesome if you could add really specific exceptions for the above cases rather than just disabling it.

[SpainTrain]

:+1:

[harai]

+1

[syabro]

+1

[mordaha]

+1

[sc0rp10]

+1

[sigmavirus24]

All +1s are interpreted as people volunteering to demonstrate the feasibility of this with a pull request. If you have something constructive to add to the conversation, please do so.

[pandeesh]

I completely agree with @IanLee1521, using this or not should be the choice of a developer. It would be nice to have the utility support this feature and let the developer decide to use or not. IMO, it would be nice to have a summary with the no of lines violated each Exyz.

[SpainTrain]

I also agree with @IanLee1521's thoughtful comments.

All +1s are interpreted as people volunteering

As I am certain you are aware, +1's exist because GH lacks a voting feature.

discuss: https://github.com/isaacs/github/issues/9 lobby: https://www.change.org/p/github-inc-add-voting-functionality-for-github-issues

General thoughts

It's pretty shocking that this issue is contentious. This feature is the rule rather than the exception (pun absolutely intended :grin:) in style checkers and linters. A non-exhaustive list of examples with this feature:

style checkers (most similar):

• rubocop
• jscs
• scss-lint

linters:

• lint (http://www.freebsd.org/cgi/man.cgi?query=lint)
• cppcheck
• pylint
• eslint
• jshint
• coffeelint

There is a good reason for this - as developers we know that edge cases come up and need to be handled. It changes nothing that PEP8 is documented as a "spec"; its implications are limited to style (as opposed to technical integration in e.g., a protocol spec or language grammar).

laziness of people who use it to pretend to be compliant

@sigmavirus24 C'mon, this is not only rude and condescending, but makes no sense. The developers who even bother to spend their time instrumenting their projects with linters are in the top percentile of non-lazy. It is also a straw-man argument - we aren't "pretending to be compliant"; we want our codebases to be stylistically consistent and PEP8 is a reasonable style guide.

demonstrate the feasibility of this with a pull request

This wording is ambiguous - does it mean that you would merge a quality PR implementing this feature?

[sigmavirus24]

As I am certain you are aware, +1's exist because GH lacks a voting feature.

Yes, but "+1"s are also perfect ways to spam and irritate people subscribed to issues. If you want to endorse the proposal, write about why. Leaving "+1" is the absolute least someone could do to indicate vague support for something in the issue (whether it be the last comment or the issue itself or anything else).

As I am certain you are aware, GitHub is free so long as we continue to use it and provide live user testing services so they can sell GitHub Enterprise. Enterprise users (who are GitHub's primary users) and private repository users do no need "voting" so the likelihood it ever appears here is slim to none. As such, it would be far more useful if people spent a couple minutes explaining why they think this is "+1" worthy. Is that so unreasonable a request?

we want our codebases to be stylistically consistent and PEP8 is a reasonable style guide.

Then I contend that you fundamentally do not want this change and that you almost certainly do not want pep8 the tool. If you want the entire codebase to be stylistically consistent, you use configuration files to make configuration changes global to the codebase. For example, you place that configuration in a tox.ini file at the root of your source tree from which you tend to run flake8.

You wouldn't then want one file (or two, or ten) to have independent configurations that override the global one. That is the antithesis of of a single stylistically consistent codebase. Then developers who "own" a module can set whatever they want and no one will want to edit that file because it's so hideously aberrant from the rest of the codebase.

The fact of the matter is that pep8 imposes some consistency on any codebase but it is not nearly as stringent or opinionated as the other tools you mention. Take for example the following

result = some_function_with_a_gloriously_long_name(
    and_some, ridiculously, long, set, of, param, eter, names,
    that, are, passed, positionally, # ...
    # etc.
    until, finally, all_is, done)

That could just as easily have been written as

result = some_function_with_a_gloriously_long_name(
    and_some, 
    ridiculously,
    long,
    set,
    of,
    param,
    eter,
    names,
    that,
    are,
    passed,
    positionally,
    # ...
    # etc.
    until,
    finally,
    all_is,
    done)

Or

result = some_function_with_a_gloriously_long_name(
    and_some, ridiculously, long, set, of, param, eter, names,
    that, are, passed, positionally, # ...
    # etc.
    until, finally, all_is, done,
)

Or

result = some_function_with_a_gloriously_long_name(
    and_some, ridiculously, long, set, of, param, eter, names,
    that, are, passed, positionally, # ...
    # etc.
    until, finally, all_is, done,
    )

Or a handful of other ways. What we get from pep8 is already not quite the consistency that we think we're going to get. We get other people's conceptions of readability and aesthetic appeal fit inside of what meager constraints pep8 already provides. What we get with this change is a slightly post apocalyptic world where each file is the wild west and no one sees any value in continuing to have pep8 run as part of the project and these once helpful comments become lint unto themselves.

(Hint there's a lot of heavy sarcasm in that last sentence since you seem to like to belittle other people's opinions, I didn't want you taking me too seriously.)

This wording is ambiguous - does it mean that you would merge a quality PR implementing this feature?

No I mean that any pull request that can show that this is feasible without breaking the API that pep8 provides to other tools, would be helpful to see. Beyond this being the antithesis of what people think it is, it will also require a drastic overhaul of pep8's internals because of how it is currently implemented.

[kleptog]

No I mean that any pull request that can show that this is feasible without breaking the API that pep8 provides to other tools, would be helpful to see.

Ok, so this is a reversal of the comment at the beginning of this issue saying a patch would not be accepted under any circumstances? Because I considered writing a patch (actually started) until I found this issue.

Beyond this being the antithesis of what people think it is,

I think you're exaggerating. I'm one of those in this thread who actually gave examples why we need this feature. It would be great if you could look at those examples and confirm you think pep8 is really doing the right thing here.

[sigmavirus24]

Ok, so this is a reversal of the comment at the beginning of this issue saying a patch would not be accepted under any circumstances?

Let me put it like this, I am very much against this. @IanLee1521 has a different opinion. I'll defer to him but I'm rather confident that this will be nightmare-ish for anyone bothering to try this and will require massively breaking changes for how pep8 works internally and in its public API as a result.

I think you're exaggerating.

I'm really not. I looked at your examples and they are examples of code-blocks where you think PEP-0008 is wrong and pep8 shouldn't be audacious enough to enforce the document. From what I've seen, developers who tend to want to align =s (for example) tend to do it everywhere where they feel it is more aesthetically pleasing. I don't see how this feature is any more pleasing to them than simply disabling the check altogether. Regarding hanging indents, it's unclear to me why one style wouldn't be acceptable for the whole code-base. If you prefer one style, write a plugin to flake8, disable the rules in pep8 that you disagree with and use your plugin to enforce your custom style.

I'm especially not here to argue whether PEP-0008 (and pep8) make code more readable. The contention is not that the style-guide itself creates readable code, the contention is that consistency internal to a project and external across a community makes code more easily consumable to a larger audience already familiar with that style. Go fmt does not necessarily make Go easier to read, it enforces consistency.

Having per-file configurations is the opposite of consistency in a codebase. If you're after consistency in your own style, asking it to be enforced by pep8 is a bit unreasonable.

[SpainTrain]

It's fair that because pep8 is less strict than many other style guides, valid edge cases ought to be more rare. However, in my case pep8 compliance breaks the code due to regrettable but necessary module shenanigans. If one uses the official method of vendoring deps on app engine (https://cloud.google.com/appengine/docs/python/tools/libraries27#vendoring), you end up with garbage like:

import vendor
vendor.add('lib')
import somelib

Which throws E402 module level import not at top of file. This particular issue is not a big deal, but the larger point is that "valid" edge cases do occur where judiciously disabling rules is reasonable.

If you're after consistency in your own style, asking it to be enforced by pep8 is a bit unreasonable.

I agree and rescind my comment on pep8 being a starting point. I'm only concerned with the edge cases where disabling one rule at the file-level is slightly more elegant than using noqa per line.

To your point @sigmavirus24, even for someone that wants this feature it may be more work to implement than it is worth. Even so, it's good to know whether this issue is "Won't Fix" or "PRs Welcome".

[sigmavirus24]

Even so, it's good to know whether this issue is "Won't Fix" or "PRs Welcome".

So I usually don't make up my mind so easily. I see that you have a very valid case for wanting to disable something in exactly one context (not exactly a file, because you may only want this exception for this one case). That said, I'm not sure that the complexity of this feature will benefit you that greatly. When pep8 added E402, I'm going to guess that it did not permit the usage of # noqa, yes? In that case, I think that was a mistake (and I may have been against adding support for it before) but I think this is a good code for which # noqa would be valuable.

This especially, is starting to win me over towards having # noqa(E402,) of some sort. (Or, in a more PyLint way: # noqa: ignore=E402.)

[danijar]

+1

[jayvdb]

I have built a rather hacky flake8 extension which allows per-file and per-line (using regex) configuration out of the source tree: https://github.com/jayvdb/flake8-putty

[lehmannro]

Sphinx developer here, if I may chime in with a supporting example why this is needed: When automatically generating a base configuration for users of Sphinx documentation (conf.py generated by sphinx-quickstart) you'd usually end up with code like this:

# This option frobnicates your documents, you shouldn't have to use it!
#frobnicate = True

Currently, there's no way to allow users to use pep8 or any of its dependents (eg. flake8) on that file. They either have to ignore the whole file and not get any checks, or manually remove the quickstart examples. See sphinx-doc/sphinx#1817.

[sigmavirus24]

@lehmannro that's not a convincing case of why that's necessary since people can exclude certain paths, e.g., docs/conf.py or docs/source/conf.py.

[lehmannro]

Well, if people don't want any warnings for that file, sure, they can ignore the whole file. If they'd still like to use pep8 for their code in that file, then that won't help.

[sigmavirus24]

My point is that there are a lot of warnings generated by a auto-generated Sphinx conf file. I've run into these as recently as this weekend starting a new one and gave up after trying to fix some of the warnings. They're not all generated by #frobnicate = True.

[The-Compiler]

FWIW I'm happy with flake8 and flake8-putty which @jayvdb mentioned earlier.

[Peque]

Just for reference. Tried with flake8-putty without success (the project seems inactive), but flake8-per-file-ignores worked for me.

Wish it was part of flake8. :smile:

[jobevers]

A case where I would have liked to be able to # noqa just one error for a file: I recently had to write code that looks like:

import os
# fixes a boto bug on GCE
# https://github.com/travis-ci/travis-ci/issues/7940#issuecomment-311411559
os.environ['BOTO_CONFIG'] = '/dev/null'

import argparse
import logging
import re
import sys

import boto.route53.connection
import boto.route53.record

and pycodestyle spit out:

6:1: E402 module level import not at top of file
7:1: E402 module level import not at top of file
8:1: E402 module level import not at top of file
9:1: E402 module level import not at top of file
12:1: E402 module level import not at top of file
13:1: E402 module level import not at top of file

[marko-ristin-parquery]

I would also second the request. Right now, we run a script to check all the *.py files in the code base and have to manually exclude the files for which we don't want certain errors reported (e.g. unit tests and component tests).

It would also make our life much easier if we could just follow what we do with pylint and have analogously:

# pydocstyle: add-ignore: D105,D107

either at the file level, or at the class or function level.

We don't use flake8, and prefer to run the checks on per-file basis.

[mwoehlke-kitware]

From what I've seen, developers who tend to want to align =s (for example) tend to do it everywhere where they feel it is more aesthetically pleasing. I don't see how this feature is any more pleasing to them than simply disabling the check altogether.

...because it should still be applied anywhere that =s are NOT aligned:

# I don't want E221 here...
sn          = 5
longer_name = 7

def foo():
    # ...but I *do* want it here
    x  = 5

# ...and here, because these aren't aligned
a    = 5
qqz = 7

---

The NUMBER ONE reason, IMHO, why we need this feature is F401 (also mentioned on SO). In general, F401 is a useful warning... but it makes absolutely no sense in an __init__.py, and is likely to be violated all over the place. In fact, it's entirely possible that the entire content of an __init__.py will consist of comments and "unused" imports. (Although, I could argue that F401 should never be applied in a file named __init__.py.)

[sigmavirus24]

@mwoehlke-kitware F401 is a check that only comes out of Flake8, so I believe your comment is in the wrong place, partly.

[tuukkamustonen]

Related: pylint is considering adding # noqa: <error> support (pylint already supports # pylint: disable=<error> syntax). Ticket https://github.com/PyCQA/pylint/issues/2493.

[sigmavirus24]

@tuukkamustonen that's tangentially related at best. Further, the proposal as linked is for a per-line basis which Flake8 already supports. Pycodestyle supports the hammer of just # noqa though.

[tuukkamustonen]

Ahem, I actually read this ticket as "per line" not "per file" :| I wasn't aware that pycodestyle already supports # noqa per-line. Docs state:

(^) These checks can be disabled at the line level using the # noqa special comment. This possibility should be reserved for special cases.

So yeah, sorry for noise.

[contang0]

File level ignoring would be very, very useful.

[sigmavirus24]

Flake8 already provides this so I'm not sure pycodestyle needs to provide this on its own as well (or attempt to reimplement that functionality)

[larsbrinkhoff]

I'd like to ignore errors from a .texthon.py file, since its syntax isn't pure Python.

[sigmavirus24]

@larsbrinkhoff you want --exclude then. Please read the documentation or review pycodestyle --help

[sigmavirus24]

Also, I'm reminded that this doesn't need to continue to stay open. We have a work-around.

[cowlinator]

@sigmavirus24 , sorry, I read this whole thread, but I'm still not aware of what the work-around is. What is the work-around?

[sigmavirus24]

Flake8 has a far more advanced and powerful system for dealing with ignoring errors on a per-file basis, per-line, etc. The work around is to use Flake8 instead of pycodestyle