ClickHouse / ClickHouse/nerve

Add ruff as a Python lint gate in CI

Open
#239 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
80
Forks
30
Avg merge
1d 14h
Merged PRs (30d)
19

Description

## Proposal

Add `ruff` as a Python lint gate in CI. Land the lint rules now; leave the tree-wide formatting rewrite as an open question, since that is the part with a real cost.

The repo has no Python lint or format tooling today: `pyproject.toml` has no `[tool.*]` sections and there is no `.pre-commit-config.yaml`. CI runs pytest and the Vite build only.

Ruff is the default choice for this: one binary in place of flake8 + isort + pyupgrade + much of pylint, with a black-compatible formatter included. CI already uses `uv`, so `uvx ruff` adds no new dependency management.

## Baseline

Measured with ruff 0.16.1 on `main` (2008b70), 311 Python files under `nerve/`, `tests/`, `scripts/`:

| Scope | Findings |
| --- | --- |
| Proposed gate — default rules (`E4`, `E7`, `E9`, `F`) plus `I` | 160 (135 auto-fixable) |
| Broader opt-in families (`B`, `SIM`, `UP`, `RUF`, `S`, `BLE`, `PERF`, …) | ~950 |
| `ruff format` | 221 of 311 files rewritten |

## Phase 1 — lint gate, now

`ruff check --fix` clears 135 of the 160, touching 84 files and 372 lines (+170/−202), nearly all of it import blocks from `I`. The remaining 25 need hands and are all small and local: 13 `F841` unused variable, 4 `E741` ambiguous name `l`, 4 `E702` semicolon statements, 3 `E731` lambda assignment, 1 `F821` undefined name.

Open branches may hit small mechanical conflicts in import order. That is a different scale from the formatter's 221-file rewrite, and it is why `I` is in this phase and the formatter is not.

The `F821` is a fair illustration of what the gate buys. `nerve/cli.py:1820` annotates `list[tuple[str, Any]]` without importing `Any`. It does not fail today, because `from __future__ import annotations` makes the annotation a string that is never evaluated — but it breaks under `typing.get_type_hints()`, and it means that annotation has never been checked by anything.

Config:

```toml
[tool.ruff]
target-version = "py313"

[tool.ruff.lint]
select = ["E4", "E7", "E9", "F", "I"]

[tool.ruff.lint.per-file-ignores]
# Re-exports in package __init__ files are intentional, not dead imports.
"__init__.py" = ["F401"]
```

The `per-file-ignores` entry is a guard rather than a fix for anything currently broken: the autofix reorders imports in seven `__init__.py` files today without removing any re-export, but without that line a future re-export would be silently stripped.

CI, as a separate job rather than a step in `backend-tests`, so a lint failure does not mask test results — same reasoning as the existing split between the fast and `slow` test steps:

```yaml
lint:
name: Lint (ruff)
runs-on: ubuntu-latest
timeout-minutes: 5
steps:
- uses: actions/checkout@v5
- uses: astral-sh/setup-uv@v6
with:
enable-cache: true
cache-dependency-glob: "pyproject.toml"
- run: uvx ruff@0.16.1 check nerve tests scripts
```

The ruff version is pinned deliberately. Rule sets change between releases, and an unpinned linter turns unrelated PRs red on a ruff release day. This matches the existing upper bounds on the `test` extra.

## Open question — `ruff format` and the broader rule families

Undecided, and worth deciding separately from phase 1.

The formatter would rewrite 221 of 311 files. There are currently 30 open PRs, including the 14-PR `config-refactor` stack (#204–#216, #224) and branches from several contributors. A rewrite that size landing on `main` forces a conflict pass through all of them, and in a stack that deep each rebase cascades into the ones above it.

Declining the formatter permanently is a legitimate answer. The gate in phase 1 works without it, and skipping it means none of the cost below is ever paid. If we do want it, it should be one commit that does nothing but reformat, scheduled for a quiet moment rather than dropped on a busy tree, and `git blame` handled the standard way:

1. Record the reformat commit in `.git-blame-ignore-revs`:
```
# ruff format,

```
2. `git config blame.ignoreRevsFile .git-blame-ignore-revs` for local blame, documented in the contributor setup notes.

GitHub's blame view reads `.git-blame-ignore-revs` from the repo root automatically, so the web UI needs no per-clone setup. Blame is the cheap part of this; the rebase pass across 30 branches is the expensive part.

## Possible follow-up — pre-commit hooks

Once CI is the enforcement point, a `.pre-commit-config.yaml` running the same pinned ruff would catch these before push rather than in review. Purely a convenience, additive, and can be added later without changing anything above.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.