ykcheungab
75 lines
4.9 KiB
Markdown
75 lines
4.9 KiB
Markdown
# Code review: the automated part
|
|
|
|
Four gates existed before this document (the test suites, `apps/studio/tools/ui_check.py`, the
|
|
real-GL smoke tests, the binding-purity test) and none of them is a linter. This adds the missing
|
|
*static* layer, deliberately small.
|
|
|
|
## Why it exists, in one measured fact
|
|
|
|
Until now the three repos had **no Ruff configuration at all**. `ruff check .` therefore ran
|
|
whatever the installed version enables by default — Ruff 0.16 enables a very broad set
|
|
(`UP`/`RUF`/`PLR`/`S`/`TRY`/`PERF`/`FURB`/`YTT`/`ASYNC`…) — so the same tree produced **1038 /
|
|
438 / 117** findings in `orisys-app` / `orisys-sdk` / `orisys-sensor-design`, and a different
|
|
Ruff version produced different ones. A gate built on that is not a gate. `ruff.toml` now pins it.
|
|
|
|
## What is gated, and what is deliberately not
|
|
|
|
| rule | meaning | gated? | why |
|
|
|---|---|---|---|
|
|
| `BLE001` | broad `except Exception` | **no** | A broad catch is the *right* choice around device I/O, GL teardown and camera probing. This codebase already records that choice inline as `# noqa: BLE001 - <reason>`. Gating it would produce ~100 findings whose only correct fix is "add a reasoned suppression". |
|
|
| `S110`, `S112` | handler is only `pass` / `continue` | **yes** | The complaint is not the breadth of the catch, it is that the failure becomes **invisible**. Silence is acceptable only with a reasoned suppression or a handler that reports/re-raises. |
|
|
| `E722` | bare `except:` | **yes** | Strictly worse than `except Exception:` — it also swallows `KeyboardInterrupt`/`SystemExit`. |
|
|
| `F` | unused import, redefinition, undefined name, unused variable | **yes** | Real defects: `F821`/`F822` are latent `NameError`s, `F811` can be a shadowed definition. |
|
|
| `B023` | closure over a loop variable | **yes** | Late binding silently produces wrong geometry in the headless Blender scripts. |
|
|
| `PLW1510` | `subprocess.run` without `check` | **yes** | A swallowed tool failure. |
|
|
| `RUF100` | unused suppression | reported, **never auto-fixed** | The `- reason` in a `noqa` comment is documentation. A machine must not delete it. Triage by hand. |
|
|
| `UP`, `I`, `PIE`, `RET501`, `PLR1711`, `SIM114`, `RUF022`, `RUF023` | mechanical modernisation | in the lint config, **not** in the gate | Applied once by `--fix`; gating them would only churn AI-authored diffs. |
|
|
|
|
`E402` (module import not at top of file) is **selected but not gated**. `import` after
|
|
`os.environ[...]` is deliberate in this codebase — the environment must be set before Qt/cv2/the SDK
|
|
load — so gating it would be pure noise. But it is the only rule that would catch an *accidental*
|
|
import moved above that setup, so it stays visible. When you touch one of those files, record the
|
|
intent as `# noqa: E402 - <reason>`; `E701`/`E702` are style, which `ruff format` owns. A gate that
|
|
cries wolf gets ignored, which is worse than no gate.
|
|
|
|
## The three commands
|
|
|
|
```powershell
|
|
# 1. The agent loop, after every edit — mechanical, no review needed.
|
|
ruff check <changed files> --fix # never --unsafe-fixes
|
|
ruff check <changed files> --output-format json # what remains, machine-readable
|
|
|
|
# 2. The gate before a change lands. Changed files only, machine-readable, non-zero on findings.
|
|
python tools/lint_gate.py # vs HEAD
|
|
python tools/lint_gate.py --base origin/main
|
|
python tools/lint_gate.py --all # gate rules over the whole repo
|
|
python tools/lint_gate.py --json
|
|
|
|
# 3. Formatting, changed files only — never a whole-tree rewrite mid-diff.
|
|
ruff format <changed files>
|
|
```
|
|
|
|
`$RUFF` may point at a specific ruff binary; otherwise `tools/lint_gate.py` uses `PATH` or the one
|
|
beside the interpreter. **Pin the version**: the gate prints the version it ran, and findings differ
|
|
between versions, so a runner must install the same one the developers use (currently 0.16.9).
|
|
|
|
## What this found when it was introduced
|
|
|
|
Filtering to rules that can indicate a defect produced ~18 candidates rather than style noise —
|
|
which is the argument for the narrow set:
|
|
|
|
- six latent `NameError`s (`np`, `sys`, `Callable`, `FrameIdentity`, `tessellate_polygon`, and an
|
|
`__all__` entry naming a function that does not exist) — none caught by any test;
|
|
- a **duplicated** function definition that silently shadowed the first;
|
|
- `lower == lower` / `upper == upper` in a range check, and the same pattern inside a **test
|
|
assertion that could never fail**;
|
|
- 13 closures over loop variables in the headless Blender/patch scripts;
|
|
- two `subprocess.run` calls whose failure was silently ignored.
|
|
|
|
## Follow-up deliberately not done yet
|
|
|
|
`PLR0124` ("name compared with itself") is not in `select`. Eleven of its sites are the deliberate
|
|
`value != value` NaN idiom and must become `math.isnan(...)` (or carry a reasoned suppression)
|
|
before the rule can be gated — otherwise it is noise. Once those are converted, add `PLR0124` to
|
|
`select` and to `GATE_SELECT` in `tools/lint_gate.py`.
|