mirror of
https://github.com/kuhyx/testsAndMisc.git
synced 2026-11-10 20:07:04 +01:00
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w2Pj4Ek3ZNMQVYcFHgXDY
This commit is contained in:
parent
ac02a237c4
commit
5029d4685b
90
docs/python-split-recipes.md
Normal file
90
docs/python-split-recipes.md
Normal file
@ -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(<module>` 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`).
|
||||
@ -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(<module>` 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 `<pkg>/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.
|
||||
|
||||
Loading…
Reference in New Issue
Block a user