Code Style Guidelines

[sivel]

Proposal: Code Style Guidelines

Author: Matt Martz <@sivel>

Date: 2017/01/26

• Status: Final
• Proposal type: core design
• Targeted Release: 2.4
• Associated PR: Add pep8 to ansible-test. ansible#20745
• Estimated time to implement: Weeks (Continually after initial phase)

This proposal was unanimously approved in the Jan 26, 2017 Testing meeting.

Motivation

Standardizing on coding guidelines are crucial to the longterm maintainability, and to our ability to act faster and more comfortably with pull requests from the community.

Problems

What problems exist that this proposal will solve?

• Code that's easier to read
• Clear guidelines when writing code
• More predictable code patterns (standardization and consistency)

Solution proposal

The errors returned from the pep8 utility were analyzed to come to a conclusion of what the current ansible code style is. This was done by counting the number of times each error was encountered, and removing those that occurred relatively few times, or were obvious anti patterns.

Existing Files

We have come to an agreement of 30 ignores for pep8, and a standard max line length of 160 characters for current files in the repository.

This will require a change to around 3390 lines over 571 files at the time of writing this. Changes will be targeted to begin once the 2.3 release is branched to stable-2.3 and devel becomes the base for the 2.4 release.

The following is the list of pep8 ignores:

E123 closing bracket does not match indentation of opening bracket's line
E124 closing bracket does not match visual indentation
E127 continuation line over-indented for visual indent
E128 continuation line under-indented for visual indent
E201 whitespace after '('
E202 whitespace before '}'
E203 whitespace before ','
E211 whitespace before '('
E221 multiple spaces before operator
E222 multiple spaces after operator
E225 missing whitespace around operator
E226 missing whitespace around arithmetic operator
E228 missing whitespace around modulo operator
E227 missing whitespace around bitwise or shift operator
E231 missing whitespace after ','
E241 multiple spaces after ','
E251 unexpected spaces around keyword / parameter equals
E261 at least two spaces before inline comment
E262 inline comment should start with '# '
E265 block comment should start with '# '
E266 too many leading '#' for block comment
E301 expected 1 blank line, found 0
E302 expected 2 blank lines, found 1
E303 too many blank lines (3)
E402 module level import not at top of file
E502 the backslash is redundant between brackets
E713 test for membership should be 'not in'
E731 do not assign a lambda expression, use a def
W391 blank line at end of file
W503 line break before binary operator

Invocation will look like:

pep8 --ignore=E123,E124,E127,E128,E201,E202,E203,E211,E221,E222,E225,E226,E228,E227,E231,E241,E251,E261,E262,E265,E266,E301,E302,E303,E402,E502,E713,E731,W391,W503 --max-line-length=160

After the initial modification to adhere to the 30 ignore ruleset, the goal with pre existing files will be to convert them to adhere to the more strict standard that new files will follow, as outlined below. The approach will be to start with the easiest modifications, by inverting --ignore to --select, totaling the number of times we see an error, and resolving those that occur the fewest times.

New Files

All new python files within the repository will adhere to a much more strict pep8 standard, identified by:

pep8 --ignore=E402 --max-line-length=160

We have decided to continue to ignore E402 module level import not at top of file due to our preference to place imports in modules below the DOCUMENTATION/RETURN/EXAMPLES/METADATA variables, as we treat those similarly to module level docstrings. This allows us to keep all code and imports together instead of split from each other.

Changes to validate-modules

To follow suit with the ignore of E402, we will update validate-modules to validate that imports are placed below documentation in a module.

Dependencies (optional)

Explain any dependencies. This section is optional but could be helpful.

• pep8
• modification to ansible-test

Testing (optional)

This extends testing capabilities

Documentation (optional)

Documentation for developing modules is required

Anything else?

To expand on the description of goals and the spirit of what we are doing here:

From PEP8:

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.

However, know when to be inconsistent -- sometimes style guide recommendations just aren't applicable. When in doubt, use your best judgment.

Of important note, is that style guidelines as defined by PEP8 are for readability and don't necessarily imply "bad programming practice".

Sometimes external style guide recommendations just aren't applicable. Based on careful and in depth discussion, we have decided to work to a point where we are explicitly ignoring 2 recommendations made by PEP8.

These exclusions are to E402 module level import not at top of file and an increase in max line length to 160 columns.

The decision to exclude E402 is based on readability. As described above, documentation strings in a module are much more similar to module level docstrings, than code, and are never utilized by the module itself. Placing the imports below this documentation and closer to the code, consolidates and groups all related code in a congruent manner to improve readability, debugging and understanding. Discussions about removing or changing how we document modules is off topic for this discussion, although can potentially impact these decisions at a later date.

The 160 column limit is largely based on larger monitors and more current development environments. This concession was decided upon quite some time ago, and was not debated for change in this proposal.

[bcoca]

• E200s i have never seen a case in which they improve readability
• E100s have to do with scope matching, those make more sense as the visual queue for a block is important
• E300s ... i can live w/o, extra blank lines annoy me but i find that sometimes people add them for a good reason, sometimes not soo good
• E713 "test for membership should be 'not in'" <= just silly
• E731 "do not assign a lambda expression, use a def" <= 99% of the time, you should not be doing either
• W391 "blank line at end of file" <= also silly
• E402 "module level import not at top of file" <= i tend to agree with this, but there are cases in which you want conditional imports

[willthames]

@bcoca conditional imports aren't affected by E402

Can we create our own rules (E402 is better than at the bottom of the file, but understand the preference to after docstrings)

[willthames]

I like all the E300s, they make it a lot easier to visually see distinct methods (although 3 blank lines rather than 2 is not so major)

W391 is just a bit careless. Also getting rid of them is probably the smallest effect on git blame you can have compared to most pep8 checks.

[jtyr]

Ignoring any of the PEP8 recommendations makes the code readability worse. Therefore full PEP8 compliance should be recommended for any new code (e.g. modules).

[jtyr]

Regarding the E402, "treat those similarly to module level docstrings" is irrelevant statement as the blocks are still variables from the Python perspective. If you want to treat them like docstrings, then turn them in real docstrings. Until then the statement "This allows us to keep all code and imports together instead of split from each other." is irrelevant as well because those are variables which makes them code as well.

[willthames]

Needs documentation update.

[willthames]

It's very annoying that flake8 and validate-modules are now incompatible.

There should be a setup.cfg that sets up flake8/pep8 for the project appropriately

[bcoca]

@willthames on E402, when the import is conditional to the code flow

if x and y:
  import q
  q(stuff)

[willthames]

@bcoca E402 does not fire for that case:

$ cat if.py
DOC = "Hello world"

import json

if False:
    import q

$ flake8 if.py 
if.py:3:1: E402 module level import not at top of file
if.py:3:1: F401 'json' imported but unused
if.py:6:5: F401 'q' imported but unused

Note that E402 fires for the non-conditional import but not the conditional import

[sivel]

Documentation updates are in process. Also we are utilizing ansible-test to run pep8 and validate-modules. If you use this test runner you should get all of the appropriate configurations.

We aren't currently using flake8, but very well may in the future.

As for flags for flake8 it should be `--ignore E402 --max-line-length 160" which are the same flags for pep8

[jtyr]

Nice, @willthames! So we can make the wrong placement of imports pass the PEP8 test by putting it into if block ;o)

DOCUMENTATION = """
...
"""

if True:
    from ansible.module_utils.basic import AnsibleModule
    from ansible.module_utils.pycompat24 import get_exception
    from ansible.module_utils._text import to_bytes

[willthames]

@sivel is there a distinction between legacy and non-legacy pep8 requirements?

tox.ini currently has 10 ignores, and I can add E402 there, but just wondered if there was a core set

[willthames]

@jtyr, that's horrible :) I think adding E402 to tox.ini will have to do.

[sivel]

@willthames see:

https://github.com/ansible/ansible/tree/devel/test/sanity/pep8

And

https://meetbot.fedoraproject.org/ansible-meeting/2017-02-02/testing_working_group.2017-02-02-17.02.log.html

I'm not at a computer right now, so just pointing in the right direction to help get you that info

[mattclay]

We don't use tox.ini for our tests, although I'm sure there are people that do use it.

[willthames]

@mattclay, it's irrelevant whether people use tox or not - if you run flake8 in a project that has a tox.ini, it picks up flake8 rules from that tox.ini.

[mattclay]

@willthames Could you submit a PR with your recommended changes to tox.ini to make it more useful for flake8 users?

[gundalow]

To summerize

• pep8 is enforced by ansible-test, and therefore CI (Shippable)
• This is documented: http://docs.ansible.com/ansible/latest/dev_guide/testing_pep8.html
• tox.ini is in sync

I think this can be marked as complete

[webknjaz]

Not sure if it's applicable here, but there's a tool, which makes easier to consolidate linters: https://pre-commit.com

[dagwieers]

We're still waiting for Powershell style validation to be included;-)

[sivel]

We're still waiting for Powershell style validation to be included;-)

Powershell linting has been started already https://github.com/ansible/ansible/tree/devel/test/sanity/pslint