Ability to change PEP8 flags on the fly in source file via inline comments #27

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet

smira commented Mar 17, 2011

Sometimes we need to violate PEP8 rules to improve readability of source file, at the same time we would like file to be still checked by PEP8.

I've added simple change that allows to influence pep8 'expected' codes:

:PEP8 -E221 -W603

a = 3
long = 4

:PEP8 +E221

Prepending minus sign disables the code, while prepending plus enables it back.

Please let me know if that makes sense for inclusion, I may need to add tests...

Add ability to parse inline options for PEP8 in source files.
Example:

 #:PEP8 -E221
 a    = 3
 bc   = 4
 #:PEP8 +E221
Contributor

reinout commented Apr 26, 2011

This one would be exactly what I need right now.

One request, though: could you rewrite the code a bit? A map and two lambda functions is all cool, but I cannot read it. I have to "decompile" it first into regular python. So make it a bit more readable.

jcrocholl: OK with merging this in principle?

rwilcox commented Apr 26, 2011

+1

smira commented Apr 27, 2011

This patch needs further cleanup: it actually silences these errors, but they still appear in stats. I can do the change and will come up with cleaner version.

Contributor

domenkozar commented Sep 18, 2011

+1

samv commented Jun 6, 2012

I feel this feature would be ripe for abuse by people who will adamantly insist that their coding style violating PEP8 is justified. I look at the example in the commit message and think, that is not more readable than the version which honors PEP8. It's ended up just being a bunch of cryptic tokens, just to let someone line up equals signs between lines. Those kinds of indentation styles are annoying when it comes to history analysis (as changing a token on one logical line affects those of the lines around it), and as useful as comments in interpretive dance.

That said, a per–file setting, something along the lines of:

#: PEP8 cleanups TODO: E12

#: PEP8 clean except: E711 E712

#: PEP8 conscientious objections:
#: E712: SQLAlchemy uses overloaded == None

This might be a good idea for a particular project; maintainers can run the pep8.py script in a way that it honors these notices, and people reading the file get a warning at the beginning of what to expect.

Contributor

domenkozar commented Jun 21, 2012

Since now we have per project configuration, is there a need for inline comments?

wkral commented Jul 13, 2012

I have an issue where I have to just import a module to include a number of decorated functions. To get rid of the warning I even tried putting them in an init function just so the import wouldn't be unused. That resulted in some errors which I found was because of lazy initialization. The only option I can see to not have this warning is put a noop function in the module and call it. I think that makes the code much worse than a comment to say this is bad but I can't think of a better option.

I definitely don't want to suppress the warning for unused imports for the whole project because that's super useful.

If I'm missing something and you have a better option for my situation I'd love to hear it.

Contributor

florentx commented Jul 14, 2012

@wkral, this report is interesting, but it does not apply to pep8 project.
Please look for pyflakes or flake8.

wkral commented Jul 14, 2012

@florentx My mistake I didn't realize that flake8 was adding on that feature I thought that it just was a wrapper that called multiple linters. Turns out that ability to suppress warnings is built in to that project. Thanks.

smira added some commits Jul 25, 2012

Merge remote-tracking branch 'upstream/master'
* upstream/master: (169 commits)
  Fix false positive E401; issue #104.
  Add tests for issue #100.
  Fix false positive for continuation line indentation when line contains comments. Closes #100
  Fix issue with bpython. Closes #101
  Back to development.
  Fix crash, and release 1.3.3; issue #98.
  Release version 1.3.2.
  Add E902 errors for IOErrors; issue #87.
  Update README.
  A little bit of rewording.
  Revert to the pre-1.2 behavior: --show-pep8 does not imply --first; issue #89.
  Fix errors on E121 and E124; issue #92.
  Add changelog entries for issue #86 and issue #91.
  Fix the default configuration path on Windows. Issue #95.
  Back to development.
  Allow 'verbose' to be specified in the config file
  Clarify .pep can be located in any parent folder
  Show maximum length in "E501 line too long" error message
  Release version 1.3.1.
  Do not read the configuration when used as a module.
  ...

Conflicts:
	pep8.py

Per project doen't solve the issue of you wanting to turn off pep8 for a single line because it's too long, but breaking it would hinder the style. E.g. a URL that's broken up can look very ugly and hard to maintain.

+1 pylint and coverage.py both have this feature. "A foolish consistency is the hobgoblin of little minds" and all that.

sbc100 commented Oct 23, 2012

+1 My specific use case it also to allow long lines that contain URLs (in my case in comments).

sbc100 commented Oct 23, 2012

@smira why not use the same syntax as pylint? e.g: "# pep8: disable=E111". In particular I think the tag should be lower case to match the name of the tool it is addressing.

Owner

sigmavirus24 commented Nov 7, 2012

I just started working with @tarekziade on flake8. Our long term goal is to make pep8 a dependency rather than something that's vendorized. I'd be willing to work with someone on @samclegg's suggested syntax for this. We have # NOQA but that will only silence pyflakes errors and warnings. This is definitely a desirable feature for plenty of users.

On the other hand, @samclegg , I feel obligated to point out that the solution to your problem is the fact that Python (like C) will concatenate adjacent strings, so if you want to do something like:

url = 'https://api.github.com/repos/sigmavirus24/Todo.txt-python/branches/master?client_id=xxxxxxxxxxxxxxxxxxxxxxxxxxxx&?client_secret=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'

You could avoid the PEP8 error by doing

url = ('https://api.github.com/repos/sigmavirus24/Todo.txt-python/branches/master'
    '?client_id=xxxxxxxxxxxxxxxxxxxxxxxxxxxx&'
    'client_secret=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx')

It's also, IMO, easier to read. That aside, I'm sure there are some much better use cases that deem this necessary.

sigmavirus24 added a commit to sigmavirus24/pep8 that referenced this pull request Nov 8, 2012

Allow users to skip a line.
This will disable pep8 for that line. It's simplistic but it works and it is
an elegant (and commonplace solution) to #27.

sbc100 commented Nov 8, 2012

I agree, but in my case the URL is in a comment which makes breaking it up less easy. Ideally
URLs in comments should be clickable/selectable.

Owner

sigmavirus24 commented Nov 8, 2012

Well check out #136 and see if that's worthwhile to you.

+1

Contributor

florentx commented Dec 18, 2012

I will merge the patch #136 which seems just enough.

Personally, I don't feel comfortable with this noise in source file, which has no added value for the reader.
I prefer to have some false positive in the PEP8 report in such case, than to add this kind of comments.
And for the E501, you can push the max_line_length to something more appropriate for your team, if needed.

Of course there's some use cases where you want a blank report. (example: http://pypi.python.org/pypi/pep8nazi)

@florentx florentx closed this Dec 18, 2012

florentx added a commit that referenced this pull request Dec 20, 2012

wimglenn commented Jul 7, 2014

This was a poor decision IMHO. The way pylint handles disables is more flexible and powerful. The current suggestion to use # noqa looks hacky, and is a workaround at best; it disables all violations when I might only want to disable a specific one. And the main reason (noise in source file) is not justified when I've seen more noise in several projects with multiple # noqa littering the source when a module-level disable of a specific code would have been appropriate. For E501, changing max_line_length for the whole project is not sufficient, it maybe the case where there is a coding standard for say 120 but there is a particular module/file in which it is more sensible to have this unlimited.

Owner

sigmavirus24 commented Jul 7, 2014

Thanks for the feedback @wimglenn

If you feel so inclined to implement what you described, feel free to send a pull request. It's unlikely to be merged, but it would be enlightening to see how you would do so. Further, if you write it, you can use it yourself (as can others who want that feature).

Contributor

florentx commented Jul 7, 2014

I do not suggest to use # noqa. I still don't like this kind of hacks even if pep8 tolerates them.

The aim of pep8 is to check compliance with some coding style rules, not to play cat and mouse with rules. I understand that your use case is special, but I don't want to add more complexity when the current options give enough flexibility for most projects.

Specifically, I'm surprised you take the example of E501, because we've already addressed some critics in version 1.5, with PR #258. I'm curious which case we missed ...

On July 7, 2014 1:36:03 PM GMT+01:00, wim glenn notifications@github.com wrote:

This was a poor decision IMHO. The way pylint handles disables is more
flexible and powerful. The current suggestion to use # noqa looks
hacky, and is a workaround at best; it disables all violations when I
might only want to disable a specific one. And the main reason (noise
in source file) is not justified when I've seen more noise in several
projects with multiple # noqa littering the source when a module-level
disable of a specific code would have been appropriate. For E501,
changing max_line_length for the whole project is not sufficient, it
maybe the case where there is a coding standard for say 120 but there
is a particular module/file where it makes sense to have this
unlimited.


Reply to this email directly or view it on GitHub:
jcrocholl#27 (comment)

Here's how I solved pep8's inability/unwillingness to be inline configurable...

https://github.com/saltstack/salt-testing/blob/develop/salttesting/pylintplugins/pep8.py

Its not a failure proof pylint plugin, but it helps...
PEP8 codes are adapted to Pylint codes, for example E123 becomes E8123...

Hope it helps you...
Pedro Algarvio @ Phone

@apeyser apeyser referenced this pull request in nest/nest-simulator May 11, 2016

Merged

Pep8ify all Python code in NEST #337

Owner

sigmavirus24 commented May 12, 2016

Wouldn't a better solution be to make pep8 configurable to respect file level suppressions?

As in something that happens once in a file and tells flake8 what to ignore in that file? No. We've furiously avoided that as best as we can. That introduces a level of complexity that no one really wants to maintain.

it's possible to create pep8 plugins

Actually only Flake8 allows you to register plugins. Flake8 does quite a bit of work with pep8 to get it to accept our plugins.

Maybe the solution is a project for a plugin that then goes into pip, and have in pep8 documentation a link to that project so that people don't come back adding issues and necrobumps (as I am)?

I have no clue what you're asking about a project going into pip. Regardless, to implement per-file supressions would require per-file StyleGuides which would then flip the way pep8 works on its head. Hence the need for a fork. I'm sorry, but the way pep8 has been architected does not allow for this since it applies a style guide to the entirety of a run rather than a style guide per file. (Typically projects have a single style guide, not N where N is the number of files in a project.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment