PEP8 coding style

[pokoli]

Hi,

It's a good practise to follow PEP8 to follow a common coding style for python projects. Currently I see that there is a lot of issues with this project (I suppose due to legacy code):

(python3)sergi@portatil ~/wsgidav » flake8 | wc -l 2318

As it will be a huge patch to fix them all, I want to discuss it first. We have to define the following things:

• Would you like to follow this convention?
• Do you think we need to add some exceptions?
• How we manage to fix all the pyflakes issues? A development branch, or PR directly to master?
• Once fixed, do you want to add a tox test to ensure the style is followed by all of the contributors?

If we reach an agreement, it must be explained on the CONTRIBUTING file, so new comers can follow this guides also.

Thanks in advance,

[mar10]

This project is originated in 2005, when PEP8 wasn't so common. I first thought about applying PEP8 when I refactored for Python 3, but decided not to do so since this is a breaking change that would force every user of the API to refactor as well.

From PEP8:

A Foolish Consistency is the Hobgoblin of Little Minds One of Guido's key insights is that code is read much more often than it is written. The guidelines provided here are intended to improve the readability of code and make it consistent across the wide spectrum of Python code. As PEP 20 says, "Readability counts".

A style guide is about consistency. Consistency with this style guide is important. Consistency within a project is more important. Consistency within one module or function is the most important.

So currently I think we should give v2.x some time to prove maturity. We may reconsider PEP8 in the future though, so I keep this issue open.

[pokoli]

I don't think PEP8 should change the API, the code should be the same it's only a mather of style. Maybe we can do it in two phases, one without api change and the second more profound with api changes.

What do you think?

[tomviner]

Perhaps it helps to split the issues flake8 raises into groups:

• purely stylistic issues that can be fixed with autopep8
  • I count 2299 --> 1365 issues, changes here
• other purely stylistic issues that could be fixed manually
• backwards incompatible changes - actually, looking through the (post autopep8) list below, I can't see any :-)

Here's a countified list of remaining issues to give an idea:

# remove line numbers / lengths to help the grouping
$ flake8 | cut -d: -f 4- | sed s/"(.* > 79 characters)"/""/ | sed 's/[[:digit:]]//g' | sort | uniq -c | sort -rn 
    670  E line too long 
    242  E block comment should start with '# '
     63  E whitespace before '('
     52  E expected  blank line, found 
     51  E unexpected spaces around keyword / parameter equals
     45  E too many blank lines ()
     28  E test for membership should be 'not in'
     18  E continuation line under-indented for visual indent
     17  E at least two spaces before inline comment
     16  F undefined name 'to_native'
     15  E module level import not at top of file
     13  E inline comment should start with '# '
     11  E expected  blank lines, found 
     10  E whitespace after '['
      8  W trailing whitespace
      8  E whitespace before ']'
      8  E expected an indented block (comment)
      8  E do not assign a lambda expression, use a def
      8  E continuation line missing indentation or outdented
      7  E too many leading '#' for block comment
      6  F 'urllib' imported but unused
      4  F redefinition of unused 'etree' from line 
      4  E whitespace before '['
      3  F local variable '_' is assigned to but never used
      3  E missing whitespace around operator
      2  W blank line contains whitespace
      2  F 'time' imported but unused
      2  F 'sys' imported but unused
      2  F local variable 'mime' is assigned to but never used
      2  F 'base' imported but unused
      2  E missing whitespace after ','
      2  E indentation is not a multiple of four
      2  E comparison to None should be 'if cond is not None:'
      1  W .has_key() is deprecated, use 'in'
      1  F 'wsgidav.wsgidav_app.WsgiDAVApp' imported but unused
      1  F 'wsgidav.wsgidav_app.DEFAULT_CONFIG' imported but unused
      1  F 'wsgidav._version.__version__' imported but unused
      1  F 'wsgidav.fs_dav_provider.FilesystemProvider' imported but unused
      1  F 'urlparse.urlparse' imported but unused
      1  F 'urllib.unquote' imported but unused
      1  F 'urllib.quote' imported but unused
      1  F 'unittest' imported but unused
      1  F undefined name 'url'
      1  F 'tempfile.gettempdir' imported but unused
      1  F 'subprocess' imported but unused
      1  F redefinition of unused 'setup' from line 
      1  F 'multiprocessing.Process' imported but unused
      1  F local variable 'sourcedir' is assigned to but never used
      1  F local variable 'res' is assigned to but never used
      1  F local variable '_res' is assigned to but never used
      1  F local variable '_locks' is assigned to but never used
      1  F local variable 'e' is assigned to but never used
      1  F local variable 'body' is assigned to but never used
      1  F 'io.BytesIO' imported but unused
      1  F 'html.escape as html_escape' imported but unused
      1  F 'cgi' imported but unused
      1  F 'base.encodebytes as base_encodebytes' imported but unused
      1  F 'base.decodebytes as base_decodebytes' imported but unused
      1  E whitespace before '}'
      1  E whitespace before ':'
      1  E whitespace after '{'
      1  E multiple spaces before keyword
      1  E multiple spaces after keyword
      1  E closing bracket does not match visual indentation

I am happy to discuss different flake8 settings that we can then set in a setup.cfg file (for example a different max line length, or certain rules we choose to ignore) and then still try to get flake8 passing with 0 issues.

I think it's worth pointing out that whereas pep8 is purely stylistic, pyflakes is there to find programming mistakes, so there's a code reliability gain to be had here too. For that reason, as well as general readability, I'd strongly prefer not to defer this issue.

[pokoli]

@tomviner great work. I will be in favour of merging at least the autopep8 changes which should not change anyhting on the code.

Also for the message you posted most issues are with comments, unused imports and white spaces with doesn't change anything in the API and should be easly fixed manually.

[mar10]

I was mainly referring to this

Function names should be lowercase, with words separated by underscores as necessary to improve readability. mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

Whitespace changes are ok, of course. (btw 80 chars max seems a bit 80s to me, maybe could be a bit more relaxed here ;-) maybe 100?

[tomviner]

Excellent, I think we're all in agreement then. I agree changing a load of interface names would be a nasty and unnecessary surprise for users.

I've submitted that autopep8 branch as https://github.com/mar10/wsgidav/pull/47

[pokoli]

@mar10 thanks for clarifying. I'm working on a PR for the manual fixing.

[pokoli]

I uploaded a WipP in https://github.com/pokoli/wsgidav/tree/pep8 There is a flake8 file which contains the missing issues to fix. Do you want me to create a PR so we can start reviewing? Otherwise I expect that the code to review will be very large.

[mar10]

Thanks! I left some comments there Mainly I would prefer 100 chars per line max, and triple quotes for docstrings Conf.py should remain as is, also compat.py (the fallback imports are important!) I would also accept if not x in y as allowed Most other changes seem to improve readability, so thanks again :-)

[mar10]

I also tried flake8 (first time :-)

> flake8 .\wsgidav\ --count --max-line-length=99 --statistics

8     E115 expected an indented block (comment)
1     E122 continuation line missing indentation or outdented
1     E124 closing bracket does not match visual indentation
15    E128 continuation line under-indented for visual indent
11    E201 whitespace after '['
9     E202 whitespace before ']'
67    E211 whitespace before '('
3     E225 missing whitespace around operator
2     E231 missing whitespace after ','
4     E251 unexpected spaces around keyword / parameter equals
15    E261 at least two spaces before inline comment
11    E262 inline comment should start with '# '
133   E265 block comment should start with '# '
5     E266 too many leading '#' for block comment
1     E272 multiple spaces before keyword
51    E301 expected 1 blank line, found 0
11    E302 expected 2 blank lines, found 1
45    E303 too many blank lines (2)
12    E402 module level import not at top of file
108   E501 line too long (100 > 99 characters)
2     E711 comparison to None should be 'if cond is not None:'
26    E713 test for membership should be 'not in'
17    F401 'wsgidav._version.__version__' imported but unused
4     F811 redefinition of unused 'etree' from line 19
27    F821 undefined name 'to_native'
5     F841 local variable '_res' is assigned to but never used
1     W601 .has_key() is deprecated, use 'in'
595

Some thoughts here:

• E713 test for membership should be 'not in' I am not yet convinced that 'a not in b' should be enforced over 'not a in b'
  
• E501 line too long (100 > 99 characters)
  even if I would expect coders to try to stick with 80 chars, we should only enforce '<= 99'
• E402 module level import not at top of file
  Havn't looked at the concrete lines, but there may be reasons to do this (e.g. fallbacks for different Python versions or optional libraries) and to prevent recursive import errors
• F841 local variable _res is assigned to but never used
  PyDev silences warnings if the variable starts with '_', which I like because it allows to have variables for debugging
• Conf.py and compat.py should have an # flake8: noqa or s.th. similar at the top

What do you think?

[tomviner]

flake8 .\wsgidav\ --count --max-line-length=99 --statistics

Oh, I had no idea about those arguments, how handy! Thanks.

E713 test for membership should be 'not in' I am not yet convinced that 'a not in b' should be enforced over 'not a in b'

They are exactly equivalent at the byte code level (they both use the not in operation), but a not in b is considered to be more readable than not a in b. I'm not overly fussed.

E501 line too long (100 > 99 characters) even if I would expect coders to try to stick with 80 chars, we should only enforce '<= 99'

I think that's fine, let's add a [flake8] section to the setup.cfg

E402 module level import not at top of file Havn't looked at the concrete lines, but there may be reasons to do this (e.g. fallbacks for different Python versions or optional libraries) and to prevent recursive import errors

One option to automatically fix these is isort, check the results here. That fixed all the E402 errors, (except the conf.py file we want to ignore) but we should check we're happy with the results on a case by case basis. I can make that branch into a PR for review?

F841 local variable res is assigned to but never used PyDev silences warnings if the variable starts with '', which I like because it allows to have variables for debugging

Anyone know how to exclude variables by name in [flake8] config? Or perhaps just add # NOQA to those lines? The reason I'm against turning the rule off, is it often points out a programming mistake where you actually meant to use that variable but forgot.

Conf.py and compat.py should have an # flake8: noqa or s.th. similar at the top

Agreed.

[mar10]

We could apply and commit all whitespace. line length, and indentation issues first (even in compat.py, but except for conf.py), and then easier review the remaining changes?

I am not dogmatic about other changes, and I agree that we should have a zero-error configuration at the end that is checked with CI.

I think that's fine, let's add a [flake8] section to the setup.cfg

Or a new file .flake8?

[mar10]

Cool, that fixed ~150 warnings:

8     E115 expected an indented block (comment)
1     E122 continuation line missing indentation or outdented
1     E124 closing bracket does not match visual indentation
15    E128 continuation line under-indented for visual indent
11    E201 whitespace after '['
9     E202 whitespace before ']'
67    E211 whitespace before '('
3     E225 missing whitespace around operator
2     E231 missing whitespace after ','
4     E251 unexpected spaces around keyword / parameter equals
13    E261 at least two spaces before inline comment
7     E262 inline comment should start with '# '
11    E265 block comment should start with '# '
5     E266 too many leading '#' for block comment
1     E272 multiple spaces before keyword
51    E301 expected 1 blank line, found 0
10    E302 expected 2 blank lines, found 1
47    E303 too many blank lines (2)
104   E501 line too long (101 > 100 characters)
2     E711 comparison to None should be 'if cond is not None:'
26    E713 test for membership should be 'not in'
17    F401 'wsgidav._version.__version__' imported but unused
4     F811 redefinition of unused 'etree' from line 17
27    F821 undefined name 'to_native'
5     F841 local variable '_res' is assigned to but never used
1     W601 .has_key() is deprecated, use 'in'
452

[mar10]

Some more fixes #52 ...

$ flake8 --count --statistics -qq
2     E111 indentation is not a multiple of four
12    E115 expected an indented block (comment)
2     E122 continuation line missing indentation or outdented
10    E128 continuation line under-indented for visual indent
1     E203 whitespace before ':'
44    E251 unexpected spaces around keyword / parameter equals
3     E261 at least two spaces before inline comment
2     E262 inline comment should start with '# '
14    E265 block comment should start with '# '
6     E266 too many leading '#' for block comment
1     E271 multiple spaces after keyword
1     E301 expected 1 blank line, found 0
98    E501 line too long (100 > 99 characters)
2     E711 comparison to None should be 'if cond is not None:'
28    E713 test for membership should be 'not in'
8     E731 do not assign a lambda expression, use a def
20    F401 'time' imported but unused
5     F811 redefinition of unused 'setup' from line 12
6     F821 undefined name 'xrange'
8     F841 local variable 'sourcedir' is assigned to but never used
8     W291 trailing whitespace
2     W293 blank line contains whitespace
1     W601 .has_key() is deprecated, use 'in'
284