[python] Use PEP-0008 compliant code headers

[modocache]

Running the Python style guide checker pep8 on the Python code headers in this repository results in the following error being emitted:

$ pep8 utils/build-script
utils/build-script:1:1: E265 block comment should start with '# '
utils/build-script:3:1: E266 too many leading '#' for block comment
utils/build-script:5:1: E266 too many leading '#' for block comment
utils/build-script:6:1: E266 too many leading '#' for block comment
utils/build-script:8:1: E266 too many leading '#' for block comment
utils/build-script:9:1: E266 too many leading '#' for block comment
utils/build-script:11:1: E265 block comment should start with '# '
utils/build-script:11:80: E501 line too long (80 > 79 characters)

The problem is that the code header used in most Python files in the repository:

1. Do not place a space in between # and the rest of the comment.
2. Contains some lines that just barely exceed the recommend length
   limit.

In addition, not all code headers in the repository follow the same template.

This commit moves all Python code headers to the following template:

# subfolder/file_name.py - Very brief description
#
# This source file is part of the Swift.org open source project
#
# Copyright (c) 2014 - 2015 Apple Inc. and the Swift project authors
# Licensed under Apache License v2.0 with Runtime Library Exception
#
# See http://swift.org/LICENSE.txt for license information
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
#
# -----------------------------------------------------------------------------
#
# This file contains stuff that I am describing here in the header and will
# be sure to keep up to date.
#
# -----------------------------------------------------------------------------

---

There are also many Python files in this repository without any code headers at all. If we can agree upon a template I'll send another pull request to add all the missing headers.

[gribozavr]

I didn't know there was a guideline about that! I am not an expert in Python, but if our scripts trigger warnings in a standard style checker tool, that is an issue (and a barrier for) contributors. This looks like a reasonable header to me, except that we lost the -*- python -*- marker for Emacs. It is required to detect the correct file type when there is no file extension. Could we re-add that?

[modocache]

Good point! Updated the commit with the following template:

# subfolder/file_name.py - Very brief description
#
# This source file is part of the Swift.org open source project
#
# Copyright (c) 2014 - 2015 Apple Inc. and the Swift project authors
# Licensed under Apache License v2.0 with Runtime Library Exception
#
# See http://swift.org/LICENSE.txt for license information
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
#
# -----------------------------------------------------------------------------
#
# This file contains stuff that I am describing here in the header and will
# be sure to keep up to date.
#
# -----------------------------------------------------------------------------
#
# -*- python -*-

[gribozavr]

@modocache Sorry, I forgot to mention that it has to be on the first line, or on the second line if the first line is a shebang. http://www.gnu.org/software/emacs/manual/html_node/emacs/Specifying-File-Variables.html

[practicalswift]

@gribozavr Below are all PEP-8 violations found in current master. Of these, which violations/classes of violations would you consider candidates for fixing?

• E101 (86 violations): indentation contains mixed spaces and tabs
• E111 (194 violations): indentation is not a multiple of four
• E121 (8 violations): continuation line under-indented for hanging indent
• E122 (15 violations): continuation line missing indentation or outdented
• E124 (1 violations): closing bracket does not match visual indentation
• E125 (8 violations): continuation line with same indent as next logical line
• E126 (4 violations): continuation line over-indented for hanging indent
• E127 (4 violations): continuation line over-indented for visual indent
• E128 (184 violations): continuation line under-indented for visual indent
• E129 (2 violations): visually indented line with same indent as next logical line
• E131 (1 violations): continuation line unaligned for hanging indent
• E201 (48 violations): whitespace after '('
• E202 (45 violations): whitespace before ')'
• E203 (64 violations): whitespace before ','
• E222 (2 violations): multiple spaces after operator
• E225 (9 violations): missing whitespace around operator
• E227 (4 violations): missing whitespace around bitwise or shift operator
• E231 (36 violations): missing whitespace after ','
• E251 (22 violations): unexpected spaces around keyword / parameter equals
• E261 (11 violations): at least two spaces before inline comment
• E262 (1 violations): inline comment should start with '# '
• E265 (51 violations): block comment should start with '# '
• E301 (4 violations): expected 1 blank line, found N
• E302 (80 violations): expected 2 blank lines, found N
• E303 (18 violations): too many blank lines (N)
• E401 (2 violations): multiple imports on one line
• E501 (198) line too long (N > 79 characters)
• E701 (6 violations): multiple statements on one line (colon)
• W191 (86 violations): indentation contains tabs
• W291 (32 violations): trailing whitespace
• W293 (65 violations): blank line contains whitespace
• W391 (6 violations): blank line at end of file

[gribozavr]

@practicalswift I'm not an expert in Python style, so let me ask a question: which of these warnings are reasonably expected to be seen a well-written Python project? Are projects typically strictly following PEP-8?

Why am I asking? Our intent is to maintain a high quality bar for the Python code. Conforming to a style guide requirements where there is no actual consensus in the community just to silence a style checker tool is not interesting.

[practicalswift]

@gribozavr The PEP 8 guidelines were formalised back in 2001 (see https://www.python.org/dev/peps/pep-0008/) and I would say that there is very broad consensus in the Python community around these recommendations. @modocache, do you agree? :-)

I would say that it is reasonable to expect that all new Python being added to the Swift project should adhere to PEP 8 (it is easily checked with the pep8/flake8 tools). The open question is if we should fix also existing code :-)

To show the changes needed to bring the code base into PEP 8 compliance I have pushed to an experimental branch https://github.com/practicalswift/swift/commits/fix-pep8-violations which fixes all PEP 8 violations (left to do: E501+ E265).

[gribozavr]

If there is broad agreement about following these guidelines, I don't see a reason why we shouldn't do so as well, especially given that there are tools that check them automatically.

[practicalswift]

@gribozavr Do you want me to submit a PR to request a merge of https://github.com/practicalswift/swift/tree/fix-pep8-violations ? If so I should probably rebase when this PR (#762) is merged.

My branch fix-pep8-violations passes utils/build-script -cRT and fixes all outstanding PEP 8 violations except E501 and E265.

@modocache, if you have time - please take a look at https://github.com/practicalswift/swift/tree/fix-pep8-violations and see if you have any suggestions on how to improve :-)

[modocache]

@gribozavr Updated with the -*- python -*- on the first or second line, forming the following template:

# subfolder/file_name.py - Very brief description -*- python -*--
#
# This source file is part of the Swift.org open source project
#
# Copyright (c) 2014 - 2015 Apple Inc. and the Swift project authors
# Licensed under Apache License v2.0 with Runtime Library Exception
#
# See http://swift.org/LICENSE.txt for license information
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
#
# -----------------------------------------------------------------------------
#
# This file contains stuff that I am describing here in the header and will
# be sure to keep up to date.
#
# ----------------------------------------------------------------------------

Below I have some findings on PEP-0008 compliance. I think this may end up being a long discussion (or bikeshed 😉), but I think this pull request should be merged in the meantime--however you feel about PEP-0008 compliance, this pull request at least standardizes the existing code headers.

---

I'm not an expert in Python style, so let me ask a question: which of these warnings are reasonably expected to be seen a well-written Python project? Are projects typically strictly following PEP-8?

This is a really interesting question. I initially strongly agreed with @practicalswift -- I was under the impression that there was broad consensus around PEP-0008. And while I do believe most people try to adhere to those guidelines, in practice it appears projects do not always conform.

As an experiment, I tried running pep8 . | wc -l against projects like the Python stdlib, the projects with the most "stars" on GitHub, and some up-and-coming projects on GitHub's trending page. I then compared these numbers to the overall number of files in the project (find . -type f | wc -l).

Project URL	Number of Files	Number of Warnings
Python 3.5.1 stdlib	1,948	71,161
Python 2.7.11 stdlib	1,980	83,227
https://github.com/jkbrzt/httpie	90	42
https://github.com/django/django	5,399	19,855
https://github.com/mitsuhiko/flask	232	465
https://github.com/tqdm/tqdm	50	2
https://github.com/ansible/ansible	1,262	9,651
https://github.com/pydata/pandas	885	30,461

There clearly aren't enough data points here, but I think it's safe to say that PEP-0008 compliance is not stringently observed in the Python community.

In addition, some maintainers actively do not follow PEP-0008; see: https://github.com/kennethreitz/requests/pull/1964#issuecomment-37736480

Here are my conclusions:

1. Whether to enforce PEP-0008 is a discussion that probably belongs on the swift-dev mailing list.
2. My vote is to conform to PEP-0008. It makes for a convenient, automatically enforceable "style guide" for our Python code. By enforcing it we may be able to minimize churn on pull requests (comments that ask contributors to write Python in one style or another, based on individual preference), and make it easier to contribute.
3. I'm typically against massive code modifications, since they pollute git blame and are difficult to review. But if there were ever a time to enforce PEP-0008 in our codebase, now would be it! I'd love to see @practicalswift's PEP-0008-compliant branch merged.

[modocache]

After reviewing https://github.com/practicalswift/swift/tree/fix-pep8-violations, I'd argue it's a huge improvement for consistency in our Python code. Just my two cents, though!

[gribozavr]

but I think this pull request should be merged in the meantime--however you feel about PEP-0008 compliance

I agree.

Below I have some findings on PEP-0008 compliance.

Very interesting data! Thank you for gathering it!

This is exactly the kind of practical evidence I was looking for. And, unfortunately, it does show that there is no broad consensus.

Whether to enforce PEP-0008 is a discussion that probably belongs on the swift-dev mailing list.

I agree.

[practicalswift]

@modocache Thanks for providing data. The consensus around PEP 8 is not as strong as I assumed :-)

@modocache, @gribozavr I think we have already covered the most severe violations (unused imports, deprecated syntax, etc.) in the current code base - the remaining changes are quite cosmetic (max 80 chars, indentation style, etc.). So let's skip them for now :-)

[practicalswift]

@modocache, @gribozavr: A thought - instead of striving for full PEP8 conformance, what about striving for no PEP8 regressions instead?

That is we accept the current classes of violations found in the repo, but we strive for not introducing any new violation types.

The following flake8 command ignores current violations and will hence only show regressions:

flake8 --ignore=E101,E111,E121,E122,E123,E124,E125,E126,E127,E128,E129,E131,E201,E202,E203,E222,E225,E226,E227,E231,E241,E251,E261,E262,E265,E301,E302,E303,E401,E501,E701,W191,W291,W293,W391 .

@modocache, if you have time it would be really interesting to see your table above but with numbers from a re-run with these ignores applied :-)

Hopefully the remaining rules after those ignores are totally non-controversial :-)