Skip to content

Adopt black formatting and enforce it in CI #85

Description

@chrislupp

Summary

black and flake8 are both declared in the dev extra of pyproject.toml, but neither has any configuration and formatting is not enforced anywhere. The result is that formatting drifts between contributors and gets argued about in review.

Adopt black with its default settings, in one mechanical pass, and enforce it in CI.

What is enforced today

.github/workflows/tests.yaml runs flake8 twice:

flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics

The first is a genuine gate, but it selects only syntax errors and undefined names. The second passes --exit-zero, so it can never fail. There is a correctness check; there is no style check.

Cost, measured

Churn across philote_mdo/, tests/ and examples/, measured with black --diff and --check (nothing written):

line length files reformatted changed lines
79 45 2880
88 (black default) 42 2451
100 49 2706

The default is the cheapest option, which is unusual and worth taking advantage of: it means no line-length key is needed in the config and there is nothing to re-litigate later.

It is cheapest because the code is already wrapped near where black would put it. Across the package excluding generated/, the 95th-percentile line is 80 characters and the 99th is 90; only 67 lines exceed 88. Most of the diff is quote normalisation and trailing commas rather than rewrapping.

Proposed sequence

Three commits in one PR. Keeping the reformat commit pure is the point -- a reviewer should be able to verify it by re-running the tool instead of reading 2451 lines.

  1. Add [tool.black] to pyproject.toml. No line-length; the default is what we want.
  2. One commit containing nothing but black . output. No other edits.
  3. Add .git-blame-ignore-revs containing that commit's SHA, and a black --check step to tests.yaml.

.git-blame-ignore-revs matters: a 42-file reformat otherwise rewrites git blame for most of the package. GitHub honours the file automatically in its blame view, and locally it is enabled with:

git config blame.ignoreRevsFile .git-blame-ignore-revs

Timing

Do this while no large branch is open. A 42-file reformat conflicts with everything in flight. feature/jobid (#76) is currently outstanding and touches the protocol, all three servers, the client and most of the test suite; this should land after it merges, not before or during.

It was considered and rejected as part of that branch: mixing a mechanical reformat into a substantive change destroys the property that makes the reformat safe to review, and saves nothing, since the conflict cost falls on whoever rebases either way.

Notes

  • No flake8 conflict to resolve. Black disagrees with flake8's E203 on slice spacing, but the blocking flake8 pass selects only E9,F63,F7,F82, and the second pass cannot fail. Black can be added without touching the flake8 configuration.
  • philote_mdo/generated/ is machine-generated and should be excluded via extend-exclude, or it will be reformatted on every compile_proto.py run and produce spurious diffs.
  • Worth considering as an alternative, though a larger decision than this issue: ruff format is black-compatible and ruff check subsumes what flake8 is doing here, at a fraction of the runtime.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentation

Type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions