From a438652aaacc3a2fcd246bfeb404a85f060c648c Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sun, 16 Aug 2026 17:07:21 +0200 Subject: [PATCH] Split brother_printer dropped-page detection out of consumables consumables.py 279 -> 217. check_page_delivery and _snapshot_counters move to _page_delivery.py. The state-file primitives stay put: test_consumables.py patches _load_consumable_state, _save_consumable_state and CONSUMABLE_STATE_FILE on consumables, and moving them would have let a test write to the real ~/.config/brother_printer/state.json. Verified by mtime after the run, not just by a green suite. test_consumables_part2.py patches the same three primitives from both sides of the seam, so it gains a second MOD_PD applied per class rather than a blanket swap. Pass count is 402 before and after. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011w2Pj4Ek3ZNMQVYcFHgXDY --- ...rinter-page-delivery-split-2026-08-16.json | 21 +++++ ...rinter-page-delivery-split-2026-08-16.json | 52 +++++++++++++ python_pkg/brother_printer/_page_count.py | 6 +- python_pkg/brother_printer/_page_delivery.py | 76 +++++++++++++++++++ python_pkg/brother_printer/consumables.py | 64 +--------------- .../tests/test_consumables_part2.py | 49 ++++++------ 6 files changed, 179 insertions(+), 89 deletions(-) create mode 100644 docs/superpowers/contracts/brother-printer-page-delivery-split-2026-08-16.json create mode 100644 docs/superpowers/evidence/brother-printer-page-delivery-split-2026-08-16.json create mode 100644 python_pkg/brother_printer/_page_delivery.py diff --git a/docs/superpowers/contracts/brother-printer-page-delivery-split-2026-08-16.json b/docs/superpowers/contracts/brother-printer-page-delivery-split-2026-08-16.json new file mode 100644 index 00000000..cc658abe --- /dev/null +++ b/docs/superpowers/contracts/brother-printer-page-delivery-split-2026-08-16.json @@ -0,0 +1,21 @@ +{ + "title": "Split brother_printer/consumables.py under the 250-line cap", + "objective": "Bring python_pkg/brother_printer/consumables.py (279 lines) under the repo-wide 250-line cap by moving the dropped-page detection into a new _page_delivery.py, while leaving every state-file primitive in consumables where two test modules patch it.", + "acceptance_criteria": [ + "consumables.py and the new _page_delivery.py are each at most 250 lines.", + "The brother_printer suite still reports exactly 402 passing tests -- the same count as before the split, proving no test was dropped or silently skipped.", + "The package keeps 100% branch coverage of the brother_printer source modules.", + "_load_consumable_state, _save_consumable_state, _get_cups_total_pages, CONSUMABLE_STATE_FILE and CUPS_PAGE_LOG stay defined in consumables.py, so test_consumables.py's patches still intercept every write that would otherwise reach ~/.config/brother_printer/state.json.", + "~/.config/brother_printer/state.json is unmodified after a full suite run, checked by mtime rather than inferred from a green run.", + "test_consumables.py needs no edit; test_consumables_part2.py gains a second MOD constant applied only to the two classes whose functions moved, never as a blanket substitution.", + "No assertion is changed and no test is deleted, skipped or weakened.", + "No suppression is added and no coverage exclusion is introduced." + ], + "out_of_scope": [ + "Changing what any test asserts, or any counter-rebasing arithmetic.", + "Moving the state-file primitives, which would risk a test writing to the live consumable state.", + "Shrinking the module docstring, which records why the printer's counter is trusted over the CUPS page log.", + "Splitting the brother_printer test files, which are separate commits." + ], + "verifier": "PYTHONPATH=. python3 -m pytest python_pkg/brother_printer/tests -q (expects exactly 402 passed), plus ls -la ~/.config/brother_printer/state.json to confirm the live state file was not written, plus pre-commit run --files on the touched files, plus bash ~/utils/scripts/check_file_length.sh --all for the cap." +} diff --git a/docs/superpowers/evidence/brother-printer-page-delivery-split-2026-08-16.json b/docs/superpowers/evidence/brother-printer-page-delivery-split-2026-08-16.json new file mode 100644 index 00000000..a022ad08 --- /dev/null +++ b/docs/superpowers/evidence/brother-printer-page-delivery-split-2026-08-16.json @@ -0,0 +1,52 @@ +{ + "intent": "Bring brother_printer/consumables.py under the 250-line cap by moving the dropped-page detection into its own module, with no behaviour change.", + "scope": [ + "python_pkg/brother_printer/_page_delivery.py (new)", + "python_pkg/brother_printer/consumables.py", + "python_pkg/brother_printer/_page_count.py (import path only)", + "python_pkg/brother_printer/tests/test_consumables_part2.py (second MOD constant, applied per class)", + "Non-goal: no assertion changed, no test deleted, skipped or weakened" + ], + "changes": [ + "Moved check_page_delivery and _snapshot_counters verbatim into python_pkg/brother_printer/_page_delivery.py, which imports the three state-file primitives from consumables.", + "The primitives themselves stay in consumables.py: test_consumables.py patches _load_consumable_state, _save_consumable_state, _get_cups_total_pages, CONSUMABLE_STATE_FILE and CUPS_PAGE_LOG on that module, and moving any of them would have let a test write to the real state file at ~/.config/brother_printer/state.json.", + "consumables.py dropped the now-unused PAGE_DROP_WARN_THRESHOLD and PageDeliveryCheck imports; 279 -> 217 lines.", + "test_consumables_part2.py gained a second constant MOD_PD alongside MOD, applied only to TestCheckPageDelivery and TestSnapshotCounters. Its other two classes test functions that did not move and keep MOD. This is the per-name discipline docs/python-split-recipes.md requires, not a blanket swap: the same three primitives are patched from both modules, so which MOD is correct depends on which caller the test exercises.", + "_page_count.py imports check_page_delivery from _page_delivery now; test_page_count.py patches that name on _page_count, so it needed no edit." + ], + "verification": [ + { + "command": "PYTHONPATH=. python3 -m pytest python_pkg/brother_printer/tests -q", + "result": "pass", + "evidence": "402 passed before the split and 402 passed after it - identical pass count, not merely a green run." + }, + { + "command": "PYTHONPATH=. python3 -c \"from python_pkg.brother_printer import _page_delivery as pd, consumables as c, _page_count as pc; pd._load_consumable_state; pd._save_consumable_state; pd._get_cups_total_pages; c.CONSUMABLE_STATE_FILE; c.CUPS_PAGE_LOG; c._migrate_state_to_printer_scale; pc.check_page_delivery\"", + "result": "pass", + "evidence": "runtime probe OK - both MOD and MOD_PD resolve every name their classes patch." + }, + { + "command": "ls -la ~/.config/brother_printer/state.json", + "result": "pass", + "evidence": "mtime still Jul 17 14:42 after the full suite run, so no test escaped its mock and wrote to the live consumable state. A green pass count alone would not have caught that." + }, + { + "command": "wc -l python_pkg/brother_printer/consumables.py python_pkg/brother_printer/_page_delivery.py", + "result": "pass", + "evidence": "217 and 76 lines, both under the 250-line cap." + }, + { + "command": "pre-commit run --files ", + "result": "pass", + "evidence": "ruff, ruff format, mypy, pylint, bandit and pytest-with-coverage all pass on the touched files." + } + ], + "risks": [ + "test_consumables_part2.py now carries two module constants; a future edit that patches a state primitive in the wrong one fails loudly with an AttributeError rather than silently, because each name exists in only one of the two modules under its own MOD.", + "consumables <- _page_delivery <- _page_count is a three-module chain; consumables must never import from _page_delivery or it becomes a cycle." + ], + "rollback": [ + "git revert the commit; consumables.py returns to 279 lines and _page_delivery.py disappears.", + "After rollback, re-run the package suite and confirm it still reports 402 passed." + ] +} diff --git a/python_pkg/brother_printer/_page_count.py b/python_pkg/brother_printer/_page_count.py index 9b3956c2..930139b7 100644 --- a/python_pkg/brother_printer/_page_count.py +++ b/python_pkg/brother_printer/_page_count.py @@ -8,6 +8,7 @@ than through ``display``. from __future__ import annotations +from python_pkg.brother_printer._page_delivery import check_page_delivery from python_pkg.brother_printer._supply import render_life_bar from python_pkg.brother_printer.constants import ( BOLD, @@ -19,10 +20,7 @@ from python_pkg.brother_printer.constants import ( YELLOW, _out, ) -from python_pkg.brother_printer.consumables import ( - check_page_delivery, - estimate_consumable_life, -) +from python_pkg.brother_printer.consumables import estimate_consumable_life def _display_page_delivery_warning(printer_total: int, *, queue_idle: bool) -> None: diff --git a/python_pkg/brother_printer/_page_delivery.py b/python_pkg/brother_printer/_page_delivery.py new file mode 100644 index 00000000..0a2da9d3 --- /dev/null +++ b/python_pkg/brother_printer/_page_delivery.py @@ -0,0 +1,76 @@ +"""Dropped-page detection: CUPS's page log versus the printer's own counter. + +Split out of :mod:`python_pkg.brother_printer.consumables` to keep it under +the 250-line cap. The state-file I/O stays in ``consumables`` because +``tests/test_consumables.py`` patches it there; it is imported here so +``tests/test_consumables_part2.py`` can patch it on this module for the two +functions that moved. +""" + +from __future__ import annotations + +from python_pkg.brother_printer.constants import PAGE_DROP_WARN_THRESHOLD +from python_pkg.brother_printer.consumables import ( + _get_cups_total_pages, + _load_consumable_state, + _save_consumable_state, +) +from python_pkg.brother_printer.data_classes import PageDeliveryCheck + + +def check_page_delivery(printer_total: int, *, queue_idle: bool) -> PageDeliveryCheck: + """Compare pages CUPS logged against pages the printer actually counted. + + Only meaningful between jobs: mid-job, CUPS has logged pages the printer has + not yet pulled off the wire, which would look identical to dropping them. + Records a fresh snapshot of both counters whenever it runs cleanly. + + Args: + printer_total: Lifetime count from the printer's own counter. + queue_idle: False when a job is queued or printing, which makes any + comparison meaningless. + + Returns: + The comparison. suspected is True only when CUPS claims materially more + pages than the printer recorded. + """ + check = PageDeliveryCheck() + if printer_total <= 0 or not queue_idle: + return check + state = _load_consumable_state() + cups_total = _get_cups_total_pages() + last_printer = state["last_printer_count"] + last_cups = state["last_cups_total"] + + _snapshot_counters(state, printer_total, cups_total) + + if last_printer <= 0 or last_cups <= 0: + # No baseline yet: this run establishes one. + return check + printer_delta = printer_total - last_printer + cups_delta = cups_total - last_cups + if printer_delta < 0 or cups_delta < 0: + # Counter reset or the page log rotated; nothing to conclude. + return check + check.cups_pages = cups_delta + check.printer_pages = printer_delta + check.dropped = cups_delta - printer_delta + check.suspected = check.dropped >= PAGE_DROP_WARN_THRESHOLD + return check + + +def _snapshot_counters( + state: dict[str, int], + printer_total: int, + cups_total: int, +) -> None: + """Persist where both counters stood, for the next run to compare against.""" + if ( + state["last_printer_count"] == printer_total + and state["last_cups_total"] == cups_total + ): + return + updated = dict(state) + updated["last_printer_count"] = printer_total + updated["last_cups_total"] = cups_total + _save_consumable_state(updated) diff --git a/python_pkg/brother_printer/consumables.py b/python_pkg/brother_printer/consumables.py index 1b0d60e5..c9e7d06e 100644 --- a/python_pkg/brother_printer/consumables.py +++ b/python_pkg/brother_printer/consumables.py @@ -20,15 +20,11 @@ from python_pkg.brother_printer.constants import ( CUPS_PAGE_LOG_PATH, DRUM_RATED_PAGES, GREEN, - PAGE_DROP_WARN_THRESHOLD, RESET, TONER_RATED_PAGES, _out, ) -from python_pkg.brother_printer.data_classes import ( - PageCountEstimate, - PageDeliveryCheck, -) +from python_pkg.brother_printer.data_classes import PageCountEstimate logger = logging.getLogger(__name__) @@ -186,64 +182,6 @@ def _cups_total_on_printer_scale(state: dict[str, int]) -> int: return cups_total + offset -def check_page_delivery(printer_total: int, *, queue_idle: bool) -> PageDeliveryCheck: - """Compare pages CUPS logged against pages the printer actually counted. - - Only meaningful between jobs: mid-job, CUPS has logged pages the printer has - not yet pulled off the wire, which would look identical to dropping them. - Records a fresh snapshot of both counters whenever it runs cleanly. - - Args: - printer_total: Lifetime count from the printer's own counter. - queue_idle: False when a job is queued or printing, which makes any - comparison meaningless. - - Returns: - The comparison. suspected is True only when CUPS claims materially more - pages than the printer recorded. - """ - check = PageDeliveryCheck() - if printer_total <= 0 or not queue_idle: - return check - state = _load_consumable_state() - cups_total = _get_cups_total_pages() - last_printer = state["last_printer_count"] - last_cups = state["last_cups_total"] - - _snapshot_counters(state, printer_total, cups_total) - - if last_printer <= 0 or last_cups <= 0: - # No baseline yet: this run establishes one. - return check - printer_delta = printer_total - last_printer - cups_delta = cups_total - last_cups - if printer_delta < 0 or cups_delta < 0: - # Counter reset or the page log rotated; nothing to conclude. - return check - check.cups_pages = cups_delta - check.printer_pages = printer_delta - check.dropped = cups_delta - printer_delta - check.suspected = check.dropped >= PAGE_DROP_WARN_THRESHOLD - return check - - -def _snapshot_counters( - state: dict[str, int], - printer_total: int, - cups_total: int, -) -> None: - """Persist where both counters stood, for the next run to compare against.""" - if ( - state["last_printer_count"] == printer_total - and state["last_cups_total"] == cups_total - ): - return - updated = dict(state) - updated["last_printer_count"] = printer_total - updated["last_cups_total"] = cups_total - _save_consumable_state(updated) - - def estimate_consumable_life(printer_total: int = 0) -> PageCountEstimate: """Estimate toner/drum life from pages printed since the last replacement. diff --git a/python_pkg/brother_printer/tests/test_consumables_part2.py b/python_pkg/brother_printer/tests/test_consumables_part2.py index 31caac88..70e9e1e8 100644 --- a/python_pkg/brother_printer/tests/test_consumables_part2.py +++ b/python_pkg/brother_printer/tests/test_consumables_part2.py @@ -9,16 +9,21 @@ from __future__ import annotations from unittest.mock import MagicMock, patch +from python_pkg.brother_printer._page_delivery import ( + _snapshot_counters, + check_page_delivery, +) from python_pkg.brother_printer.consumables import ( STATE_SCHEMA_CUPS_SCALE, STATE_SCHEMA_PRINTER_SCALE, _cups_total_on_printer_scale, _migrate_state_to_printer_scale, - _snapshot_counters, - check_page_delivery, ) MOD = "python_pkg.brother_printer.consumables" +# check_page_delivery and _snapshot_counters moved to _page_delivery, so the +# state-file helpers they call resolve through that module's globals instead. +MOD_PD = "python_pkg.brother_printer._page_delivery" class TestMigrateStateToPrinterScale: @@ -113,9 +118,9 @@ class TestCheckPageDelivery: never actually came out. """ - @patch(f"{MOD}._snapshot_counters") - @patch(f"{MOD}._get_cups_total_pages", return_value=1841) - @patch(f"{MOD}._load_consumable_state") + @patch(f"{MOD_PD}._snapshot_counters") + @patch(f"{MOD_PD}._get_cups_total_pages", return_value=1841) + @patch(f"{MOD_PD}._load_consumable_state") def test_detects_dropped_pages( self, mock_load: MagicMock, @@ -133,9 +138,9 @@ class TestCheckPageDelivery: assert check.dropped == 63 assert check.suspected is True - @patch(f"{MOD}._snapshot_counters") - @patch(f"{MOD}._get_cups_total_pages", return_value=1841) - @patch(f"{MOD}._load_consumable_state") + @patch(f"{MOD_PD}._snapshot_counters") + @patch(f"{MOD_PD}._get_cups_total_pages", return_value=1841) + @patch(f"{MOD_PD}._load_consumable_state") def test_healthy_job_does_not_warn( self, mock_load: MagicMock, @@ -150,9 +155,9 @@ class TestCheckPageDelivery: assert check.dropped == 0 assert check.suspected is False - @patch(f"{MOD}._snapshot_counters") - @patch(f"{MOD}._get_cups_total_pages", return_value=1780) - @patch(f"{MOD}._load_consumable_state") + @patch(f"{MOD_PD}._snapshot_counters") + @patch(f"{MOD_PD}._get_cups_total_pages", return_value=1780) + @patch(f"{MOD_PD}._load_consumable_state") def test_small_gap_is_not_a_warning( self, mock_load: MagicMock, @@ -175,9 +180,9 @@ class TestCheckPageDelivery: def test_skipped_without_printer_count(self) -> None: assert check_page_delivery(0, queue_idle=True).suspected is False - @patch(f"{MOD}._snapshot_counters") - @patch(f"{MOD}._get_cups_total_pages", return_value=1841) - @patch(f"{MOD}._load_consumable_state") + @patch(f"{MOD_PD}._snapshot_counters") + @patch(f"{MOD_PD}._get_cups_total_pages", return_value=1841) + @patch(f"{MOD_PD}._load_consumable_state") def test_first_run_only_establishes_baseline( self, mock_load: MagicMock, @@ -189,9 +194,9 @@ class TestCheckPageDelivery: assert check.suspected is False mock_snap.assert_called_once() - @patch(f"{MOD}._snapshot_counters") - @patch(f"{MOD}._get_cups_total_pages", return_value=10) - @patch(f"{MOD}._load_consumable_state") + @patch(f"{MOD_PD}._snapshot_counters") + @patch(f"{MOD_PD}._get_cups_total_pages", return_value=10) + @patch(f"{MOD_PD}._load_consumable_state") def test_log_rotation_draws_no_conclusion( self, mock_load: MagicMock, @@ -206,9 +211,9 @@ class TestCheckPageDelivery: check = check_page_delivery(2087, queue_idle=True) assert check.suspected is False - @patch(f"{MOD}._snapshot_counters") - @patch(f"{MOD}._get_cups_total_pages", return_value=1841) - @patch(f"{MOD}._load_consumable_state") + @patch(f"{MOD_PD}._snapshot_counters") + @patch(f"{MOD_PD}._get_cups_total_pages", return_value=1841) + @patch(f"{MOD_PD}._load_consumable_state") def test_printer_counter_reset_draws_no_conclusion( self, mock_load: MagicMock, @@ -224,7 +229,7 @@ class TestCheckPageDelivery: class TestSnapshotCounters: - @patch(f"{MOD}._save_consumable_state") + @patch(f"{MOD_PD}._save_consumable_state") def test_writes_new_snapshot(self, mock_save: MagicMock) -> None: state = {"last_printer_count": 2000, "last_cups_total": 1700} _snapshot_counters(state, 2087, 1778) @@ -232,7 +237,7 @@ class TestSnapshotCounters: assert saved["last_printer_count"] == 2087 assert saved["last_cups_total"] == 1778 - @patch(f"{MOD}._save_consumable_state") + @patch(f"{MOD_PD}._save_consumable_state") def test_unchanged_counters_skip_the_write(self, mock_save: MagicMock) -> None: """Do not rewrite the file on every run when nothing moved.""" state = {"last_printer_count": 2087, "last_cups_total": 1778}