From 5029d4685b33896c19e7d92195593d0fc25e8ae4 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sun, 16 Aug 2026 16:29:45 +0200 Subject: [PATCH] Split the handoff doc and refresh its stale counts The handoff had grown to 264 lines -- a violation of the very cap it exists to enforce. The Python recipes move to docs/python-split-recipes.md; the handoff keeps a pointer. Also refreshed what had gone stale: the directory distribution said python_pkg 50 and '48 near-misses' from before this session's 9 splits. A future session reading a stale count would not know which other claims in the file to trust, so the numbers are now explicitly labelled a snapshot with the script as the authority, and the per-package breakdown points at brother_printer (15) and code_tutor (13) as where the bulk now sits. Corrected the test recipe too: shared builders go in tests/conftest.py, not a _helpers.py -- name-tests-test rejects the latter, which the earlier wording would have walked the next session straight into. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011w2Pj4Ek3ZNMQVYcFHgXDY --- docs/python-split-recipes.md | 90 +++++++++++++++++++ refactor_claude_todo_resume.md | 156 +++++++++------------------------ 2 files changed, 131 insertions(+), 115 deletions(-) create mode 100644 docs/python-split-recipes.md diff --git a/docs/python-split-recipes.md b/docs/python-split-recipes.md new file mode 100644 index 00000000..b3c0aefd --- /dev/null +++ b/docs/python-split-recipes.md @@ -0,0 +1,90 @@ +# Python split recipes for the 250-line cap + +Settled empirically while clearing `python_pkg/wsg_grabber`. Every rule here +cost at least one failed gate run or one revert to learn — do not re-derive +them. Referenced from `refactor_claude_todo_resume.md`. + +## The re-export form + +When a split moves a **public** name out of a module that callers/tests still +reach through the old path, the re-export must be marked with `__all__`: + +```python +from python_pkg.pkg._new_module import show_logs, summarise + +__all__ = ["build_parser", "main", ..., "show_logs", "summarise"] +``` + +The PEP 484 alias form `from x import y as y` **does not work here**: ruff +raises `PLC0414` (useless-import-alias), auto-fixes it away, and then removes +the now-"unused" import under `F401` — silently breaking `cli.summarise`. +Verified on `wsg_grabber/cli.py`; `__all__` passes ruff + mypy + pylint clean. + +Splits that only move **private** helpers (leading `_`, no external caller) +need no shim at all — check with grep first, as in `catalog.py`. + +`__all__` only needs the **re-exported** names, not every public name in the +module — names defined locally stay reachable either way. Verified against the +full gate on `store.py`. + +**`__all__` does not save you from ruff's `TC001`.** It suppresses `F401`, but +a re-exported class used only in annotations gets rewritten into the +`TYPE_CHECKING` block, which lints perfectly and raises `AttributeError` at +import time. This nearly shipped a broken `ui.Callbacks` that `cli.py` +constructs. A class that must be reachable at runtime from module X stays +_defined_ in module X. **Always finish a re-export split with** + +```bash +PYTHONPATH=. python3 -c "from python_pkg.pkg import mod; mod.TheName" +``` + +Two other things the gate will not catch for you: + +- **Check what the tests patch before choosing a seam.** `test_ui.py` does + `patch.object(ui, "tk", fake)`, so every line touching `tkinter` had to stay + in `ui.py`; moving widget construction out errored 22 tests. Grep the test + file for `patch.object(` first and treat those names as pinned. +- **Tests that call private methods pin them too** — `test_player.py` calls + `mpv._note_errors(...)`, so that method stayed as a one-line delegate to the + moved implementation. + +**Budget the new import block and `__all__` before deciding a two-way split is +enough.** Five splits so far landed 2–16 lines over the cap after the first cut +and needed a further seam; the header costs 10–20 lines that the arithmetic on +function sizes does not show. + +Splitting a **class**: if tests call private methods as bound attributes +(`worker._fetch_thread(...)` — grep the tests first), module-level functions +will not do; use mixins. The shape that passes every gate with **no +suppression** (settled on `downloader.py`, do not re-derive): + +```python +# _worker_base.py +class WorkerBase(ABC): + _deps: WorkerDeps # bare annotations for read-only attributes + _downloaded: int = 0 # real default for any attribute the mixin ASSIGNS + + @abstractmethod + def _publish(self, event: DownloadEvent) -> None: + """Supplied by the concrete Worker.""" +``` + +Both mixins inherit `WorkerBase`; `Worker(ScanMixin, DownloadMixin)`. Why not +the obvious alternatives: + +- Naming a class `*Mixin` does **not** get you pylint's `no-member` exemption + here — `ignored-checks-for-mixins` is not configured for it. Don't rely on it. +- `raise NotImplementedError` stub bodies are **uncovered lines** under the + 100% branch gate. +- `if TYPE_CHECKING: def _publish(...) -> None: ...` satisfies mypy but leaves + pylint reporting `no-member`. +- A bare annotation (`_downloaded: int`) is not enough for an attribute the + mixin assigns to (`+= 1`); pylint wants a real default. + +Watch the MRO: `Worker(ScanMixin, DownloadMixin)` means ScanMixin wins any +name collision. + +Coverage: `meta/pyproject.toml` sets `source = ["python_pkg"]`, so a new +sibling module is measured automatically. Confirmed: new modules show up in +the report at 100% rather than being silently skipped. Add no new branches +(no `if TYPE_CHECKING` guards beyond what moved, no defensive `try/except`). diff --git a/refactor_claude_todo_resume.md b/refactor_claude_todo_resume.md index bd585852..e77fcc48 100644 --- a/refactor_claude_todo_resume.md +++ b/refactor_claude_todo_resume.md @@ -13,25 +13,16 @@ present. Done already (do not redo): -| Commit | What | -| --------- | -------------------------------------------------------------------- | -| `~/utils` | `plans/` + `sessions/` exempted from the cap (`specs/` still capped) | -| `17b20e5` | New `no-inline-python` pre-commit hook + the one violation fixed | -| `004b4b0` | `CLAUDE.md` 259 → 141 lines | -| `72a5e28` | `poker-stakes/` tracked in git (58 files) | -| `73b85f5` | `poker-stakes/` 4 violations → 0 | -| `77c6d9f` | `wsg_grabber/catalog.py` 251 → 175 | -| `8ce4536` | `wsg_grabber/cli.py` 282 → 247 | -| `62ad148` | `wsg_grabber/review.py` 330 → 196 | -| `262ef38` | `wsg_grabber/net.py` 335 → 109 | -| `f58caea` | `wsg_grabber/store.py` 382 → 202 | -| `324ec0c` | `wsg_grabber/downloader.py` 420 → 238 (mixins) | -| `16c6266` | `wsg_grabber/player.py` 373 → 235 | -| `d86965f` | `wsg_grabber/ui.py` 398 → 242 (mixin) | +- `~/utils`: `plans/` + `sessions/` exempted from the cap (`specs/` still capped) +- `17b20e5` new `no-inline-python` pre-commit hook + its one violation +- `004b4b0` `CLAUDE.md` 259 → 141 +- `72a5e28` + `73b85f5` `poker-stakes/` tracked, then 4 violations → 0 +- `77c6d9f`…`d86965f` **every `wsg_grabber` source file** — `catalog` 251→175, + `cli` 282→247, `review` 330→196, `net` 335→109, `store` 382→202, + `downloader` 420→238, `player` 373→235, `ui` 398→242 +- `e03fbd1` `wsg_grabber/tests/test_review.py` 297 → 112 + two new modules -**Every `wsg_grabber` source file is now under the cap.** What remains in that -package is its 7 test files, which are the next thing to do — see the note on -shared fixtures under "Traps". +`git log --oneline --grep='250-line cap'` is the authoritative list. ## The live worklist @@ -41,11 +32,19 @@ Do not work from a list in this file — it goes stale. Run: bash ~/utils/scripts/check_file_length.sh --all ``` -Current distribution: `linux_configuration` 84, `python_pkg` 50, -`phone_focus_mode` 13, `kcd2_dice_solver` 12, `focus_owner` 11, `billsplit` 6, -`reverse_survivors`/`meta`/`docs` 4 each, `.github`/`bucket_catch` 2 each. -**48 of them are near-misses at 251–300 lines** — highest count-drop per unit -of effort, do these first. +**Every count in this file is a snapshot and will be wrong by the time you read +it. Run the script.** As of `ac02a23` the shape was `linux_configuration` 84, +`python_pkg` 41, `phone_focus_mode` 13, `kcd2_dice_solver` 12, `focus_owner` +11, `billsplit` 6, `reverse_survivors`/`meta`/`docs` 4 each, +`.github`/`bucket_catch` 2 each — quoted only to tell you where the bulk sits, +never to work from. + +Within `python_pkg`: `brother_printer` 15, `code_tutor` 13, `wsg_grabber` 6 +(tests only), then singles. + +**Do the source files in a package before its tests.** Proven on `wsg_grabber`: +the test files then split along seams the source already established, instead +of you inventing a new boundary for them. ## Decisions already made (do not re-ask) @@ -84,90 +83,13 @@ Plus `.github/workflows/file-length.yml` modelled on `.utils`), scoped to changed files. Then prove it fails: stage a deliberately 251-line file, confirm `git commit` aborts, delete it. -## The Python re-export form — settled empirically, do not re-derive +## Python split recipes -When a split moves a **public** name out of a module that callers/tests still -reach through the old path, the re-export must be marked with `__all__`: - -```python -from python_pkg.pkg._new_module import show_logs, summarise - -__all__ = ["build_parser", "main", ..., "show_logs", "summarise"] -``` - -The PEP 484 alias form `from x import y as y` **does not work here**: ruff -raises `PLC0414` (useless-import-alias), auto-fixes it away, and then removes -the now-"unused" import under `F401` — silently breaking `cli.summarise`. -Verified on `wsg_grabber/cli.py`; `__all__` passes ruff + mypy + pylint clean. - -Splits that only move **private** helpers (leading `_`, no external caller) -need no shim at all — check with grep first, as in `catalog.py`. - -`__all__` only needs the **re-exported** names, not every public name in the -module — names defined locally stay reachable either way. Verified against the -full gate on `store.py`. - -**`__all__` does not save you from ruff's `TC001`.** It suppresses `F401`, but -a re-exported class used only in annotations gets rewritten into the -`TYPE_CHECKING` block, which lints perfectly and raises `AttributeError` at -import time. This nearly shipped a broken `ui.Callbacks` that `cli.py` -constructs. A class that must be reachable at runtime from module X stays -_defined_ in module X. **Always finish a re-export split with** - -```bash -PYTHONPATH=. python3 -c "from python_pkg.pkg import mod; mod.TheName" -``` - -Two other things the gate will not catch for you: - -- **Check what the tests patch before choosing a seam.** `test_ui.py` does - `patch.object(ui, "tk", fake)`, so every line touching `tkinter` had to stay - in `ui.py`; moving widget construction out errored 22 tests. Grep the test - file for `patch.object(` first and treat those names as pinned. -- **Tests that call private methods pin them too** — `test_player.py` calls - `mpv._note_errors(...)`, so that method stayed as a one-line delegate to the - moved implementation. - -**Budget the new import block and `__all__` before deciding a two-way split is -enough.** Five splits so far landed 2–16 lines over the cap after the first cut -and needed a further seam; the header costs 10–20 lines that the arithmetic on -function sizes does not show. - -Splitting a **class**: if tests call private methods as bound attributes -(`worker._fetch_thread(...)` — grep the tests first), module-level functions -will not do; use mixins. The shape that passes every gate with **no -suppression** (settled on `downloader.py`, do not re-derive): - -```python -# _worker_base.py -class WorkerBase(ABC): - _deps: WorkerDeps # bare annotations for read-only attributes - _downloaded: int = 0 # real default for any attribute the mixin ASSIGNS - - @abstractmethod - def _publish(self, event: DownloadEvent) -> None: - """Supplied by the concrete Worker.""" -``` - -Both mixins inherit `WorkerBase`; `Worker(ScanMixin, DownloadMixin)`. Why not -the obvious alternatives: - -- Naming a class `*Mixin` does **not** get you pylint's `no-member` exemption - here — `ignored-checks-for-mixins` is not configured for it. Don't rely on it. -- `raise NotImplementedError` stub bodies are **uncovered lines** under the - 100% branch gate. -- `if TYPE_CHECKING: def _publish(...) -> None: ...` satisfies mypy but leaves - pylint reporting `no-member`. -- A bare annotation (`_downloaded: int`) is not enough for an attribute the - mixin assigns to (`+= 1`); pylint wants a real default. - -Watch the MRO: `Worker(ScanMixin, DownloadMixin)` means ScanMixin wins any -name collision. - -Coverage: `meta/pyproject.toml` sets `source = ["python_pkg"]`, so a new -sibling module is measured automatically. Confirmed: new modules show up in -the report at 100% rather than being silently skipped. Add no new branches -(no `if TYPE_CHECKING` guards beyond what moved, no defensive `try/except`). +`docs/python-split-recipes.md` holds the settled Python rules: the `__all__` +re-export form, the `TC001` trap that lints clean and breaks at runtime, the +mixin shape for splitting a class, what the tests pin in place, and the +coverage behaviour of new sibling modules. **Read it before the first Python +split** — each rule there cost a failed gate run or a revert to establish. ## Split recipes @@ -178,15 +100,19 @@ the report at 100% rather than being silently skipped. Add no new branches API stable by re-exporting. - **TS/Dart** — extract components; re-export from the original path so no importer changes. -- **Tests** — split by describe/test-group. Coverage must not drop. - In `python_pkg`, `meta/pyproject.toml` omits `*/tests/*` from coverage **by - path**, so a shared `/tests/_helpers.py` is coverage-safe — the opposite - of the `poker-stakes` trap below, where fixtures beside the source got counted - as production code. Prefer one shared helper module over another `partN` file: - the repo already carries `test_cups_service_part2/part3`, - `test_consumables_part2` and `test_downloader_part2`, and every new `partN` - that redeclares the same fixture scaffolding pushes jscpd toward its 2% - duplication threshold, which only fires on the real `git commit`. +- **Tests** — split by describe/test-group. Coverage must not drop, and + **verify by the pass _count_, not a green run**: a split that loses a whole + module still reports "passed". + In `python_pkg`, shared fixture builders go in the package's + **`tests/conftest.py`** — the `name-tests-test` hook rejects any other + non-`test_*.py` file under `tests/`, so a `_helpers.py` will fail the gate. + `python_pkg/syncyomi_guard/tests/conftest.py` is the precedent, and + `meta/pyproject.toml` omits `*/tests/*` from coverage by path so nothing + there is counted as production code (the opposite of the `poker-stakes` + trap below). Prefer one shared conftest over another `partN` file: the repo + already carries four, and each new one that redeclares the same fixture + scaffolding pushes jscpd toward its 2% duplication threshold — which only + fires on the real `git commit`. Never game the cap: no one-lining, no deleting tests, no moving code into an exempt extension, no suppressions.