Since we're likely to be doing some license header changes in the near future, I'm thinking that it might be a good time to start discussing code style for our projects.
I'd like to be pretty much pep8 unless there are good reasons to deviate from that standard. PEP8 is well known and tools to check against it are widely available.
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.
Tim
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.
How about:
[FORMAT] # Maximum number of characters on a single line. max-line-length=100
/me has no problem with 80 chars, but most of my monitors can easily handle two 100-char on one vertically split-view in vim (and 80 chars is sometimes quite a pain)
J.
How about:
[FORMAT] # Maximum number of characters on a single line. max-line-length=100
/me has no problem with 80 chars, but most of my monitors can easily handle two 100-char on one vertically split-view in vim (and 80 chars is sometimes quite a pain)
Traitor!
I can't really fit 100 columns my in favorite python editor (Spyder) on my 12-inch screen that I use when working from bed^H^H^Hhome. I can fit 74 columns by default, or 90 columns if I configure the widget layout in a sub-optimal way.
But if there's a strong desire for more columns, I'll manage. Can't hinder the team, can I? :)
But if there's a strong desire for more columns, I'll manage. Can't hinder the team, can I? :)
Also, we should mention that by default the maximal line length is set to 79, not 80.
Let's just set it to 80 (as we already use it in the code), and forget about the heretic 100 idea :)
But if there's a strong desire for more columns, I'll manage. Can't hinder the team, can I? :)
Also, we should mention that by default the maximal line length is set to 79, not 80.
Let's just set it to 80 (as we already use it in the code), and forget about the heretic 100 idea :)
Yeah, I would definitely support 80 instead of default 79, good point :-)
But I don't really object to using 90, if people wish. I'd avoid anything larger, if possible.
On Tue, 3 Jun 2014 09:44:42 -0400 (EDT) Kamil Paral kparal@redhat.com wrote:
How about:
[FORMAT] # Maximum number of characters on a single line. max-line-length=100
/me has no problem with 80 chars, but most of my monitors can easily handle two 100-char on one vertically split-view in vim (and 80 chars is sometimes quite a pain)
Traitor!
I can't really fit 100 columns my in favorite python editor (Spyder) on my 12-inch screen that I use when working from bed^H^H^Hhome. I can fit 74 columns by default, or 90 columns if I configure the widget layout in a sub-optimal way.
But if there's a strong desire for more columns, I'll manage. Can't hinder the team, can I? :)
It amuses me that you're strongly "80 columns only" for code but are pretty anti-column-width-restriction on email :)
In general, I'm fine with the 80 column restriction on code but I'd like it not to be an absolute restriction. There are just somethings that end up being more readable with relaxed column width guidelines (some strings, hyperlinks etc.).
Would you be OK with ignoring some longer lines assuming that we have a tool that can ignore those violations in a sane way?
Tim
But if there's a strong desire for more columns, I'll manage. Can't hinder the team, can I? :)
It amuses me that you're strongly "80 columns only" for code but are pretty anti-column-width-restriction on email :)
Yeah, it's a bit ironic. It's mostly about available tools and how they (fail to) work. Like Zimbra.
But the core ideological difference is that email is text, and there's no reason not to wrap it/re-flow it. OTOH the visual style is essential for code, and can't be wrapped/re-flowed (that makes it very unreadable).
Is there someone who has issues with reading my long unwrapped lines? What are the issues? Are there any email clients which can't autowrap lines? I don't really know about any.
In general, I'm fine with the 80 column restriction on code but I'd like it not to be an absolute restriction. There are just somethings that end up being more readable with relaxed column width guidelines (some strings, hyperlinks etc.).
Would you be OK with ignoring some longer lines assuming that we have a tool that can ignore those violations in a sane way?
Sure, makes sense to sometimes ignore the restriction. I'm just not sure if it can be configured somehow, i.e. I assume we will see those lines permanently as false negatives.
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 =====
libtaskotron/directives/bodhi_comment_directive.py:247:67: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:247:69: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:293:13: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:293:15: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:293:31: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:293:33: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:426:28: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:426:30: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:427:33: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:427:35: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:428:29: E251 unexpected spaces around keyword / parameter equals libtaskotron/directives/bodhi_comment_directive.py:428:31: E251 unexpected spaces around keyword / parameter equals
def _is_comment_needed(old_result, comment_time, result, time_span = None):
-or-
outcome = _post_testresult(self.bodhi_api, detail.item, env_data['checkname'], detail.outcome, url = "http://example.com", doreport = input_data['doreport'], arch = detail.keyvals.get('arch', 'noarch'), )
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 :)
==== 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
raise exc.TaskotronValueError("'output' parameter must be a " "mutable sequence of strings. Yours was: %s" % type(output))
This is a braindead check. You have a limited line length, and it is happens that you need to type the opening bracket shortly before the end of the line, PEP8 imagines you end up providing the rest of the code as a short column of text underneath it. Like this:
raise exc.TaskotronValueError("'output' parameter must be a " "mutable sequence of strings. " "Yours was: %s" % type(output))
or you break the line immediately after the opening bracket, like this:
raise exc.TaskotronValueError( "'output' parameter must be a " "mutable sequence of strings. Yours was: %s" % type(output))
Still, the most readable code I believe is the first one, against PEP8.
==== E303 ====
libtaskotron/check.py:88:5: E303 too many blank lines (2) libtaskotron/check.py:110:5: E303 too many blank lines (2) libtaskotron/check.py:115:5: E303 too many blank lines (2) libtaskotron/check.py:122:5: E303 too many blank lines (2) libtaskotron/check.py:142:5: E303 too many blank lines (2) libtaskotron/check.py:148:5: E303 too many blank lines (2) libtaskotron/check.py:165:5: E303 too many blank lines (2) libtaskotron/check.py:189:5: E303 too many blank lines (2) libtaskotron/check.py:224:5: E303 too many blank lines (2)
This forbids you from adding two blank lines between class methods. Another braindead checks.
==== 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.
==== 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.
==== 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.
==== 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.
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.
First of all - forget the max-line-length comment from earlier... I went through the pep8's configuration options, and there is basically next to none, so the overall decision will mostly need to be "keep it or drop it".
==== 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 :)
I can, of course, try and change my mindset, but it is true, that i find it more readable the way I'm used to write it
==== E303 ====
This forbids you from adding two blank lines between class methods. Another braindead checks.
Sadly, this can not be configured in any way.
==== E124 ====
In a longer line of text, having the closing bracket really matching the opening one (the first example) is much easier on the eyes.
Agreed!
==== E122 =====
I don't really understand the purpose of this error. If I move 'type(' to the third line, it goes away.
I must say, that I find the "proper" way enforced by pep8 to be a bit more readable/understandable. It makes sense, but we can IMHO easily dismiss this error.
==== E111, E113 ====
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.
There is an option to disable checks using #noqa comment, but (sadly) not for these errors.
All in all I'd be for configuration that ignores at least E128 and E303. My personal preference would be also ignoring E303 and E124. I have no strong opinion on E122, although I'd vote for keeping it, should it ever come to it. E111 and E113 should IMHO be kept, even if it means this kind of false positives.
J.
On Tue, 3 Jun 2014 09:29:49 -0400 (EDT) Kamil Paral kparal@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 differentupdates: '%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 differentupdates: '%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 containcorrect '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 URLMe 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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 06/04/2014 02:01 PM, Tim Flink wrote:
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.
One thing to watch with pep8 is it has the notion of "ignored by default" errors. If you start turning additional errors off, you need to make sure to turn those ones off as well.
(my own major gripe is with E121, since that was based on a misreading of the PEP, which has since been clarified: https://github.com/jcrocholl/pep8/issues/256)
Cheers, Nick.
- -- Nick Coghlan Red Hat Hosted & Shared Services Software Engineering & Development, Brisbane
Testing Solutions Team Lead
==== 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 differentupdates: '%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 differentupdates: '%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 containcorrect '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 URLMe 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/qa-devel
qa-devel@lists.fedoraproject.org
-
Josef Skladanka -
Kamil Paral -
Nick Coghlan -
Tim Flink