Add per file disabling of warnings

[IanLee1521]

Per "suggestion" of @dubglan in #264 (comment), 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 👎 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]

👍

[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.