Skip to content

Add ruff and rumdl to enforce style on this repo #40

Description

@gaurav

Nothing enforces style here today: no ruff config, no rumdl config, and .github/workflows/tests.yml runs pytest only. This is to add both and wire them into CI.

It already pays for itself

Running ruff for the first time found a real latent bug, not just style:

tests/test_db_download.py:68:30: F821 Undefined name `Path`
tests/test_db_download.py:73:38: F821 Undefined name `Path`
tests/test_db_download.py:79:37: F821 Undefined name `Path`

_FakeEnsure annotates three signatures with Path and never imports it. The tests pass because from __future__ import annotations makes annotations strings that are never evaluated — so this is invisible until something evaluates them (typing.get_type_hints, a runtime validator, or simply dropping the future import). That is exactly the class of error nobody catches by reading.

Already fixed in bc2b76d, confirmed real rather than cosmetic: before the fix typing.get_type_hints(_FakeEnsure.ensure) raised NameError: name 'Path' is not defined; after it the three hints resolve to pathlib.Path. Counts as evidence for this issue, not against it — the linter found it on its first run, before any config existed.

Current state, measured

Tool Finding
ruff check . 24 errors, 8 auto-fixable (was 27; bc2b76d fixed all three F821s with one import)
ruff format --diff . 15 files would be reformatted
rumdl check . 56 issues in 5 files, 11 auto-fixable
lines > 88 chars 101, across src/ and tests/

Top ruff rules by count in what remains: PLW1510 subprocess-run-without-check (5), ISC004 implicit-string-concatenation (4), BLE001 blind-except (3), I001 unsorted-imports (3).

Decisions this needs before it is a mechanical change

  • Line length. 101 lines exceed ruff's default 88. The code here is deliberately comment-heavy and several of those are prose in docstrings, so the choice is a longer limit (100?) or reflowing a lot of explanation. Picking 88 and auto-formatting would rewrap comments that were laid out by hand.
  • rumdl's MD013 at 80 columns. This repo's Markdown is hard-wrapped at ~80 already, but tables, code blocks and long links legitimately exceed it — 56 issues is mostly this. Either raise the limit or exempt tables/code.
  • Which ruff rules. The 27 above are ruff's defaults plus what it surfaces with a broad selection; BLE001 and PLW1510 in particular are judgement calls in test code, where a bare except or an unchecked subprocess.run is often deliberate.
  • ruff format or not. Adopting the formatter is a one-time large diff that will bury git blame for every file. Worth doing, but worth doing as its own commit so it can be added to .git-blame-ignore-revs.

Suggested order

  1. Land a pyproject.toml config for both with the decisions above.
  2. One commit of pure auto-fixes (ruff check --fix, ruff format, rumdl fmt), separate and blame-ignorable.
  3. One commit fixing what is left by hand — including the F821 above, which should arguably be fixed on its own before any of this.
  4. Add a lint job to .github/workflows/tests.yml, last, so CI is green the moment it appears.

Doing (4) first would put a red X on every PR until the rest lands.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions