YTEP-0037: Code styling


Created: May 18, 2020 Author: Clément Robert

This YTEP proposes the enforcement of code styling guidelines with auto-formatting tools.



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


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.


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)

On the meeting

  • merge isort pass on the code base + CI check + doc (done)
  • merge (needs tweaking) #2598 (done)
  • merge blackening + manual fixups + CI checks + doc (done)
  • signal to open PR authors that they should apply black (see transitioning strategy)

Can be done later

  • merge #2600 (done)
  • merge #2595 (done)
  • reduce flake8 ignore list (done)
  • add bugbear plugin and correct detected anti-patterns

Backwards Compatibility



  • 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.