On Tue, 3 Jun 2014 09:29:49 -0400 (EDT)
Kamil Paral <kparal(a)redhat.com> wrote:
> Outside any header requirements or directive documentation
> requirements, are there any changes to PEP8 that folks want to
> make? If so, please list the exceptions and why you think they
> should be adopted.
==== E251 =====
Josef is used to add spaces between keyword name and its value in
method definitions or method calls. Personally, I find it more
readable according to PEP8 (with no space), but Josef claims the
opposite :)
Personally, I agree with Josef on this one. My habit is to put the
spaces in there and I tend to find it a bit more readable but I don't
have any really strong feelings on it and have been training myself to
not put the spaces in there.
==== E128 ====
libtaskotron/check.py:94:17: E128 continuation line under-indented
for visual indent libtaskotron/check.py:97:21: E128 continuation line
under-indented for visual indent libtaskotron/check.py:209:21: E128
continuation line under-indented for visual indent
<snip>
Still, the most readable code I believe is the first one, against
PEP8.
In my opinion, the first example is the least readable of the three and
I really don't like having code formatted like that.
==== E303 ====
libtaskotron/check.py:88:5: E303 too many blank lines (2)
This forbids you from adding two blank lines between class methods.
Another braindead checks.
Eh, I'm not all that partial to either point of view. They're different
ways of looking at things.
==== E124 ====
libtaskotron/bodhi_utils.py:146:25: E124 closing bracket does not
match visual indentation
log.warn("The build %s is a part of two different
updates: '%s'" " and '%s'. That's probably a bug in
Bodhi." % (build,
failures[build]['title'],
build2update[build]['title']) )
Because PEP8 wants this:
log.warn("The build %s is a part of two different
updates: '%s'" " and '%s'. That's probably a bug in
Bodhi." % (build,
failures[build]['title'],
build2update[build]['title']) )
In a longer line of text, having the closing bracket really matching
the opening one (the first example) is much easier on the eyes.
Honestly, I don't really notice things like this. I had to stare at the
two examples for a while before I realized what the difference was. I'm
fine with keeping this restriction
==== E122 =====
libtaskotron/runner.py:127:17: E122 continuation line missing
indentation or outdented
raise TaskotronYamlError("Input yaml should contain
correct 'input'" "section (a mapping). Yours was: %s" % type(
self.taskdata['input']))
I don't really understand the purpose of this error. If I move
'type(' to the third line, it goes away.
I agree with Josef on this one. In general, I don't like having
calls hat start on one line like that and continue onto the next line
and am fine with keeping this enabled.
==== E265 ====
libtaskotron/directives/bodhi_comment_directive.py:422:13: E265 block
comment should start with '# '
#TODO: replace url with proper URL
Me and Josef are used to avoid the space in these cases. We can
re-learn, of course. But it's very common to write it this way.
Yeah, I had to relearn that habit as well but I'm of the opinion that
having a space between '#' and the rest of the comment does make many
comments easier to read.
==== E111, E113 ====
libtaskotron/config_defaults.py:53:35: E111 indentation is not a
multiple of four libtaskotron/config_defaults.py:53:35: E113
unexpected indentation
bodhi_posting_comments_span =
4320 #: # 3 days (3*24*60 = 4320)
This is a nice example how automated style checks impede visual
coding. In this case, it makes absolute sense to have the comment
written in this way. However, if we suppress E111 and E113 globally,
we might miss some potential problems elsewhere. I'm not sure if we
can suppress certain warnings only in a selected block of code, but
if we check for PEP8 automatically, we should find out. There will be
more use cases like this one.
The '#:' is for making sphinx format the comments differently when
rendering docs, no? Why can't the comment explaining that 3 days is
4320 minutes be inline?
Why is making the indentation a clean multiple of 4 not an option here?
Granted, that wouldn't fix both errors but it would fix one of them.
Sooo, I'm not exactly a big fan of automated PEP8 checking (I had
to
disable it in Spyder, because it was driving me mad), but it might be
somewhat useful, if we configure some exceptions to the general rules.
What about using pylint instead of the pep8 tool. While I realize it
isn't 100% of what pep8 covers, they're close and AFAIK, pylint is
quite a bit more configurable than pep8 compliance checkers are.
Outside of any one (or more than one) person's dislike of particular
pep8 rules, it is a standard and there is some value in not deviating
from well-known standards too much.
Of the concerns you raised, I'm not terribly opposed to most of them
other than E128 and E122. I'm not crazy about dropping E265, though.
Tim