Coding Style

[Tim Flink]

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>