[FEP02] Enforcing PEP8 rules (pep8-naming, flake8-import-order)

Vanuan commented 4 years ago

FEP02
Title	Enforcing PEP8 rules (flake8, flake8-import-order, pep8-naming)
Status	Draft
Author(s)	Vanuan
Created	Aug 28, 2020
Updated
Issue
Discussion	https://forum.freecadweb.org/viewtopic.php?t=37405
Implementation	(Partial) https://github.com/FreeCAD/FreeCAD/pull/3772

Abstract

This is a proposal to enforce PEP8 rules gradually, rule by rule, workbench by workbench using the flake8 tool.

This proposal focuses primarily on fixing the following styling errors:

pep8-naming: N801,N802,N803,N804,N805,N806,N807,N811,N812,N813,N814,N815,N816,N8017
flake8-import-order: I100, I101, I201, I202

But the approach could be extended to add more PEP8 rules.

Motivation

When reviewing Python code it's important that the coding style is consistent. If it's inconsistent, reviewer spend time teaching contributors about the coding style. Contributors, OTOH are frustrated that there's an apparent discretion that reviewers apply when reviewing the code. Different workbenches have different rules, e.g. Path/CAM, so even reviewers can't always agree on the rules a specific codebase should follow. This takes unneeded discussions, effort and energy that would've better be spent elsewhere.

The problem above means there should be some objective set of rules enforced by an automated system. Thankfully, Python community already though about these issues and prepared PEP8 - a style guide for Python code and a corresponding tool to enforce them - Flake8.

Specification

The rules are described pretty well in both the original PEP8 document and in the dedicated packages: flake8-import-order and pep8-naming.

But let's duplicate those:

Imports

I100: Your import statements are in the wrong order. I101: The names in your from import are in the wrong order. I201: Missing newline between import groups. I202: Additional newline in a group of imports.

Naming

N801 | class names should use CapWords convention N802 | function name should be lowercase N803 | argument name should be lowercase N804 | first argument of a classmethod should be named ‘cls’ N805 | first argument of a method should be named ‘self’ N806 | variable in function should be lowercase N807 | function name should not start and end with ‘__’ N811 | constant imported as non constant N812 | lowercase imported as non lowercase N813 | camelcase imported as lowercase N814 | camelcase imported as constant N815 | mixedCase variable in class scope N816 | mixedCase variable in global scope N817 | camelcase imported as acronym

Rationale

Flake8 is a de-facto standard for style guide verification, so that choice is quite straightforward.

Alternatives

No alternatives I'm aware of. There's a codespell tool but that focuses on the proper use of English (automating spelling error correction).

Backwards Compatibility

To preserve compatibility, when renaming we should add a proxy name and mark it as deprecated and put a version in which it was deprecated. After a new version of FreeCAD is released, the deprecated name may be safely removed.

To make flake8 happy, we add the following comment:

# noqa: N802

Where N802 is a name of an error we wish to ignore (until a new version of FreeCAD is released after which we can remove the ignore).

Sample Implementation

First, install the packages: python3 -m pip install flake8 flake8-import-order pep8-naming flake8-pyi

Add the .flake8 config file at the root of the project:

[flake8]
application-import-names = FreeCAD,FreeCADGui,Draft,Part,draftutils, DraftGeomUtils, DraftVecUtils
import-order-style = google
select = N801,N802,N803,N804,N805,N806,N807,N811,N812,N813,N814,N815,N816,N8017,E303,I201,I202,I101,I100
max-line-length = 160
filename = */src/Mod/Draft/draftguitools/gui_snapper.py

And run this command: flake8.

By changing the filename and select options we can configure the scope of the rule enforcement and improve the enforcement coverage file by file, rule by rule, workbench by workbench.

FreeCAD already uses Travis CI to run C++ tests. Having an additional step of Python-specific tests should not make it slower as it takes several seconds rather than minutes. If TravisCI is an issue, we can try CircleCI or other tools that give some free quota to improve the running time.

Related forum threads and wiki pages:

FAQ

Q: Is it compatible with rules we have in XXX workbench? A: The goal of this proposal is to enforce the PEP-8 rules we choose across all the workbenches. As long as these rules are supported by flake8, yes

Q: What if I don't like the rule applied in my specific case? A: You can add the line that violates the rule to ignore it (# noqa: XXX) or you could disable it in the whole file. Let's discuss on a case by case basis.

Q: But this hurts my creativity, I can't express myself in my unique style! A: FreeCAD is a collaborative project, so it's important that the coding style is consistent even if some contributors don't like it. You read the code much more often than you write it, so it takes some time to get used to. Moreover, once you learn this coding style, you can contribute to any Python project as PEP8 rules are a de-facto standard in the Python community.

Q: But can this be not enforced by the tool? I promise that I won't violate any rules! A: Unfortunately, people make mistakes. Even in a good faith, contributors can't be trusted. Tools are objective and even if they fail, they fail consistently, so issues can be fixed in bulk.

Copyright

All FEPs are explicitly CC0 1.0 Universal.

vocx-fc commented 4 years ago

When looo started this repository I wanted to open a FEP like this one but I never did because of priorities elsewhere, so I'm happy it is being proposed now.

Related threads:

Vanuan commented 4 years ago

Thanks, made the announcement at these topics too.

Vanuan commented 4 years ago

Moved to wiki. Let's do discussions on the forum

FreeCAD / FreeCAD-Enhancement-Proposals