Coding Style
Tim Flink
tflink at redhat.com
Wed Jun 4 04:01:16 UTC 2014
On Tue, 3 Jun 2014 09:29:49 -0400 (EDT)
Kamil Paral <kparal at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.fedoraproject.org/pipermail/qa-devel/attachments/20140603/1a31f05f/attachment.sig>
More information about the qa-devel
mailing list