Coding Style

Wed Jun 4 12:09:42 UTC 2014

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

I don't want to sound too negative or something, but I'm afraid I'll continue to use the first style, and Josef is of the same opinion. In these contentious cases we'll need to exclude the check.

> > ==== 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

I'm not :-) If you don't see any difference, we don't need to keep the check, I'd say.

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

I see a bit inconsistent that you don't like the opening bracket at the end of the line in the 'type(' example, but you like it after 'TaskotronValueError(' from E128:

            raise exc.TaskotronValueError(
                "'output' parameter must be a "
                "mutable sequence of strings. Yours was: %s" % type(output))

I don't really mind this check, let's keep it enabled, I just fail to see the difference.

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

No problem here.

> 
> 
> > ==== 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?

Yes, '#:' tells sphinx to include this attribute in documentation. You can also use '#: attribute description'.

What do you mean by "inline"? I can't put two different comments on the same line. And if I put it on a different line and align it, I get E113 always and E111 in 3 of 4 cases.
Sure, I don't have to align it with the value, I can use zero indentation, but sometimes the extra indentation makes so much clearer what you're talking about. Like this:

                                   # foo is an odd multiple of 5
if foo is None or self.bar(foo) or (foo % 2 == 1 and foo % 5 == 0):

This would also receive E113 and probably E111.

These check themselves are very reasonable, but they should ignore comments and unfortunately they don't.

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

Sure, in this case I can shuffle it a bit, even though it won't be cleanly aligned afterwards. But if I need to lower code readability just to satisfy a check that doesn't make any sense in that particular scenario, I'm not exactly happy about that.

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

The check that we should definitely run is pyflakes. pylint is nice as well, but we'll need to configure it (for example keep all 'error' and 'warning', but exclude many 'refactor' and 'convention' messages). pep8 is the least useful one, I think, but still some of its messages are pretty good. For example, I just learned that I shouldn't use

if not item in container:

and rather use

if item not in container:

which is pretty neat. We'll just need to spend some time with it and exclude the contentious checks.

> 
> 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
> 
> 
> 
> _______________________________________________
> qa-devel mailing list
> qa-devel at lists.fedoraproject.org
> https://admin.fedoraproject.org/mailman/listinfo/qa-devel
> 