> ==== 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(a)lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/qa-devel