YTEP-0037: Code styling¶
Abstract¶
Created: May 18, 2020 Author: Clément Robert
This YTEP proposes the enforcement of code styling guidelines with auto-formatting tools.
Status¶
Completed
Detailed Description¶
Code styling guidelines are already presented in the project’s documentation, though enforcing them is not explicitly made part of the reviewing process.
We already use flake8 and integrate it to our CI to catch a subset of
infractions to PEP 8. From
flake8’s pypi page
- Flake8 is a wrapper around these tools:
- PyFlakes
- pycodestyle
- Ned Batchelder’s McCabe script
From black’s documentation
The rules for horizontal whitespace can be summarized as: do whatever makes pycodestyle happy. (…) The coding style used by Black can be viewed as a strict subset of PEP 8.
so it is expected that black plays nicely with flake8 by construction.
black applies an opinionated style, and offers very little configuration options
by design. Only the target line length can be changed. This makes it a critical
point, requiring discussion if this YTEP is approved.
Maximal line length¶
Note
After discussion a maximum-line-length of 88, which is black’s default setting,
was adopted.
The guidelines states that
Line widths should not be more than 80 characters
Despite this being respected in most of the code base, there remains a large amount of outliers, that would be time-consuming to go through by hand. Taking the example of the yt-4.0 branch at the time of writing, there are 2158 lines exceeding 80 characters (~1.5% of the whole code base), or, visually
Note that when long strings are present, black will not attempt to split them to
shorten the individual lines.
This is most important in the case of dosctrings, and I explore the tools available to
validate them hereafter (see additional rules).
There is a range of possible values we might give preference to. Python’s standard
library caps line-length at 79, pandas does so at 88. By default, black will target
88, as its authors claim it reduces the total number of lines by some 10% (as compared
to enforcing 80).
In first drafting the PR linked above, I chose a line-length of 100, so as to minimize
the amount of manual tweaking left to me after a black pass. I estimated that
imposing a strict limit to 80 chars would leave 545 lines to be manually updated, while
capping at 88 leaves a mere 135 (75% less work). As a reference, dask uses black’s
default settings, and allows flexibility for docstrings up to 120 characters through
flake8.
Sorting imports¶
PEP8 recommends sorting imports statements,
Needless to say, the task is daunting and definitely not worthy of anyone’s time if we
had to go back and apply those rules manually to the code base.
Luckily, isort is able to check for and auto-apply those rules, so it can easily be
added to the CI-linting process.
Additional rules & flake8 plugins¶
Since the oldest python version supported (as of yt 4.0dev) is 3.6, it means we can start
using fstrings instead of str.format() and % formatting.
#2605 demonstrates how a transition can
be performed using flynt.
–
flake8-bugbear is a flake8 plugging
that goes beyond code style and detects some additional anti-patterns, most of which are
correspond very likely to design flaws in otherwise syntactically valid statement. For
instance, it will catch mutable default values such as in
def spam(a={}, b=[]):
#...
which, in most contexts, should be rewritten as
def spam(a=None, b=None):
if a is None:
a = {}
if b is None:
b = []
This is a well known “gotcha”, as documented for instance here. In short, this plugin detects bugs that went under the radar up to now, so it’s probably worth adding it to our linting CI.
–
Another plugging can be added to enforce docstring formatting (flake8-docstrings), and has a straight-forward option configuration to validate docstrings are numpy-styled. However, there is currently a very large debt in errors caught by this tool, and no way to automatically solve them. However, it could still be added to our linting CI, if check for new errors only, such as
git diff upstream/master -u -- "*.py" | flake8 --diff
(snippet borrowed from pandas’ contributing guide)
Side effects¶
Although some default options in isort conflict with black’s opinionated standard,
it can be configured so that the tools play nicely with each other.
This is demonstrated in #2596 where both
check pass on Travis.
On another note, black only recognizes pyproject.toml as a configuration file (and
is explicitly not planning to support other files such as setup.cfg).
An undesirable effect of using pyproject.toml solely as a configuration file for
black is that pip will detect it and change its behaviour when its present. The
correct way to introduce this file is by specifying yt’s build requirements within it.
A proof of concept for this is #2598,
where CI builds are run correctly across all tested python versions (3.6, 3.7, 3.8).
A serious counter-argument to applying black is that it implies messing up with git
blame by making a single contributor the de facto last-author of a large number of
lines they have not even necessarily read. Most recent versions of git can be
configured to ignore specific commits in git blame. However, black’s own README
currently points out that GitHub’s UI for git-blame does not support this feature
(yet ?).
It should be noted that black does not have a parser for Cython files, but
interestingly flake8 and isort do. Thus it is possible to add style checks for
Cython extensions to the CI pipeline.
Additionally, black will not force line-length limits in docstrings.
flake8 will still be able to catch violations there, but solving them
require manual tweaking. However, the amount of existing docstrings going over
88 characters is fairly small (a few dozens), so this is by no means a blocking
condition.
Outreach and transition¶
Enforcing these change throughout future contributions can be done by
- updating the Developer Guide (done in part in #2592)
- offering a precommit hook configuration file to help contributors automate the linting stage locally (
precommit_hook.yaml) such a configuration file is proposed in #2600
It is expected that transitioning to the “blackened” version of the code will add a bit
of overhead in merging pre-existing PRs. Specifically, a simple git merge <pr-branch>
master will almost certainly raise git conflicts. One possible solution to this is to
sanitize the pr-branch (on author side) with:
pip install lint_requirements.txt
black yt/
isort .
git merge --strategy ours master
git push
I tested this strategy locally by simulating blackening at an arbitrary point in the past and merging the current state of the code base back in, producing a net zero diff with a direct blackening of the current state. In practice I advise caution, and sanitized code should be reviewed before merging. Another, arguably cleaner way to to resolve conflicts is to rebase the branch onto master and solve conflicts along the process. This is the prefered method though I would not recommend it to contributors who are not used to rebasing since it is easy to make mistakes in the process.
The shorter the transition, the easier, so I think that most of the PRs could be merged in a very narrow time window (a day or two), provided the appropriate conditions. However, because we want to ensure that each step passes the tests, which typically takes a least an hour or two per step, I propose that prep steps be done separately, and the big one (blackening) happen on a meeting.
Roadmap¶
To ensure cohesion in getting the number of features included in this PR in the codebase, we will have a dedicated maintainer/triage meeting. This YTEP’s PR, the yt slack, and the yt-dev mailing list will include the meeting details for interested parties to attend. Some items require completion before the triage meeting, and some can be done afterwards, and have been categorized below.
Pre meeting¶
The following questions should be resolved
* settle on a maximal line length (final: 88 characters)
* decide on where should unyt import statement lands (for isort sorting): on third party section, or a custom
intermediate section between first and third parties ? (final: third party)
Backwards Compatibility¶
Yes.
Alternatives¶
- Enforcing styling guidelines through peer review for each PR. Obviously this is a lot more work. Additionally, this methodology is prone to error and may cause delay in the PR approval process in case the authors disagree with the reviewers on the application of styling rules.
- Leaving code style decisions up to authors, and embracing the style diversity.