Coding Style

[Kamil Paral]

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