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.
- Add
[tool.black] to pyproject.toml. No line-length; the default is what we want.
- One commit containing nothing but
black . output. No other edits.
- 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.
Summary
blackandflake8are both declared in thedevextra ofpyproject.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
blackwith its default settings, in one mechanical pass, and enforce it in CI.What is enforced today
.github/workflows/tests.yamlruns flake8 twice: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/andexamples/, measured withblack --diffand--check(nothing written):The default is the cheapest option, which is unusual and worth taking advantage of: it means no
line-lengthkey 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.
[tool.black]topyproject.toml. Noline-length; the default is what we want.black .output. No other edits..git-blame-ignore-revscontaining that commit's SHA, and ablack --checkstep totests.yaml..git-blame-ignore-revsmatters: a 42-file reformat otherwise rewritesgit blamefor most of the package. GitHub honours the file automatically in its blame view, and locally it is enabled with: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
E203on slice spacing, but the blocking flake8 pass selects onlyE9,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 viaextend-exclude, or it will be reformatted on everycompile_proto.pyrun and produce spurious diffs.ruff formatis black-compatible andruff checksubsumes what flake8 is doing here, at a fraction of the runtime.