diff --git a/docs/superpowers/evidence/brother-printer-display-partial-2026-08-16.json b/docs/superpowers/evidence/brother-printer-display-partial-2026-08-16.json new file mode 100644 index 00000000..4ea0ab6a --- /dev/null +++ b/docs/superpowers/evidence/brother-printer-display-partial-2026-08-16.json @@ -0,0 +1,41 @@ +{ + "intent": "Reduce python_pkg/brother_printer/display.py by extracting the parts no test patches, and collapse the duplicated toner/drum bar rendering into one helper. This does NOT bring display.py under the 250-line cap -- it is 342 and needs a further ~92 lines removed, which is blocked on a decision about editing test files.", + "scope": [ + "python_pkg/brother_printer/display.py", + "python_pkg/brother_printer/_supply.py (new)", + "python_pkg/brother_printer/_severity.py (new)", + "Non-goal: no assertion changed, no test deleted or weakened" + ], + "changes": [ + "Extracted the supply-level arithmetic into _supply.py: _classify_percentage_level, _classify_supply_level, _format_supply_bar, _process_supply_item, _parse_supply_value, _collect_supply_items. These are imported by the tests, not patched, so they were free to move.", + "Extracted the three _SEVERITY_* lookup tables into _severity.py. Pure data, patched by nothing.", + "Added _supply.render_life_bar and used it for both the toner and drum bars in _display_page_count_estimate. Those two blocks were the same logic with different colours and notes -- this removes the duplication rather than just relocating it.", + "display.py re-exports the moved supply names through __all__, because tests/test_display.py imports them from display.", + "display.py 427 -> 342 lines; _supply.py 131; _severity.py 44." + ], + "verification": [ + { + "command": "python3 -m pytest python_pkg/brother_printer/tests/ -q", + "result": "pass", + "evidence": "402 passed in 1.76s -- the same count as before the change, run after each of the three extractions." + }, + { + "command": "pre-commit run --files python_pkg/brother_printer/display.py python_pkg/brother_printer/_supply.py python_pkg/brother_printer/_severity.py", + "result": "pass", + "evidence": "ruff, ruff format, mypy, pylint, bandit and the coverage-gated pytest all pass. Only the evidence-artifact hook remained, which this file satisfies. No suppression added; a pylint C0104 on the name 'bar' was fixed by renaming it, not disabling the check." + }, + { + "command": "bash ~/utils/scripts/check_file_length.sh --all", + "result": "fail", + "evidence": "display.py is still 342 lines and remains a violation. Stated explicitly: this commit reduces it and removes real duplication, but does not clear it." + } + ], + "risks": [ + "The remaining ~92 lines cannot be extracted without either editing test files or adding ~8 pure-indirection delegate functions. brother_printer's test modules patch roughly 90 names across the package via patch(f'{MOD}.'), and mock.patch pins a name to the module it is patched on: a function that moves to a new module resolves its collaborators through the new module's globals, so the patch no longer reaches it. An earlier attempt to move the page-count block was reverted for exactly this reason (AttributeError: module display does not have the attribute 'estimate_consumable_life').", + "_display_report_header is patched from both test_display.py and test_display_part2.py, so any split of display.py into USB and network halves has to keep that function reachable from both patch sites." + ], + "rollback": [ + "git revert the commit; display.py returns to 427 lines and the two new modules are deleted.", + "After rollback, run python3 -m pytest python_pkg/brother_printer/tests/ -q and confirm it still reports 402 passed." + ] +} diff --git a/python_pkg/brother_printer/_severity.py b/python_pkg/brother_printer/_severity.py new file mode 100644 index 00000000..043be851 --- /dev/null +++ b/python_pkg/brother_printer/_severity.py @@ -0,0 +1,44 @@ +"""The icon, colour and closing summary for each PJL severity. + +Split out of :mod:`python_pkg.brother_printer.display` to keep it under the +250-line cap. Pure data: no test patches these tables, so unlike most of +``display`` they were free to move. +""" + +from __future__ import annotations + +from python_pkg.brother_printer.constants import ( + BOLD, + CYAN, + GREEN, + RED, + RESET, + YELLOW, +) + +_SEVERITY_ICONS: dict[str, str] = { + "ok": "✓", + "info": "i", + "warn": "⚡", + "critical": "⚠", +} +_SEVERITY_COLORS: dict[str, str] = { + "ok": GREEN, + "info": CYAN, + "warn": YELLOW, + "critical": RED, +} +_SEVERITY_SUMMARIES: dict[str, str] = { + "ok": f"{GREEN}{BOLD}✓ Printer is healthy. No replacements needed.{RESET}", + "info": ( + f"{CYAN}{BOLD}i Printer is busy/processing. No replacements needed.{RESET}" + ), + "warn": ( + f"{YELLOW}{BOLD}⚡ WARNING: Maintenance will be needed" + f" soon.{RESET}\n{YELLOW} Order replacement parts" + f" now to avoid interruption.{RESET}" + ), + "critical": ( + f"{RED}{BOLD}⚠ ACTION REQUIRED: Replacement or fix needed now!{RESET}" + ), +} diff --git a/python_pkg/brother_printer/_supply.py b/python_pkg/brother_printer/_supply.py new file mode 100644 index 00000000..580e73f0 --- /dev/null +++ b/python_pkg/brother_printer/_supply.py @@ -0,0 +1,131 @@ +"""Turning a raw SNMP supply reading into a percentage, a bar and a verdict. + +Split out of :mod:`python_pkg.brother_printer.display` to keep it under the +250-line cap. Only the arithmetic lives here. The functions that *print* +(``_display_supply_levels``, ``_display_supply_warnings``) stay in ``display`` +because ``tests/test_display_part2.py`` patches them there, and a patched name +has to remain resolvable in the module the test names. + +``display`` re-exports everything below, because the tests import these names +from there rather than from here. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from python_pkg.brother_printer.constants import ( + BOLD, + GREEN, + PROGRESS_BAR_WIDTH, + RED, + RESET, + SNMP_LEVEL_LOW, + SNMP_LEVEL_OK, + SUPPLY_LOW_PCT, + SUPPLY_WARN_PCT, + YELLOW, + _out, +) +from python_pkg.brother_printer.data_classes import SupplyStatus + +if TYPE_CHECKING: + from python_pkg.brother_printer.data_classes import NetworkResult + + +def _classify_percentage_level(desc: str, pct: int) -> tuple[int, str, str, str, bool]: + """Classify a supply by its calculated percentage.""" + if pct <= SUPPLY_LOW_PCT: + return pct, f"{pct}%", RED, f"{desc} at {pct}%.", True + if pct <= SUPPLY_WARN_PCT: + return pct, f"{pct}%", YELLOW, f"{desc} at {pct}% -- order soon.", False + return pct, f"{pct}%", GREEN, "", False + + +def _classify_supply_level( + desc: str, max_val: int, level: int +) -> tuple[int, str, str, str, bool]: + """Classify a supply level. Returns (pct, status, color, warning, replace).""" + if level == SNMP_LEVEL_OK: + return -1, "OK", GREEN, "", False + if level == SNMP_LEVEL_LOW: + return -1, "LOW", RED, f"{desc} is LOW.", True + if level == 0: + return 0, "EMPTY", RED, f"{desc} is EMPTY -- replace now!", True + if max_val > 0: + pct = min(level * 100 // max_val, 100) + return _classify_percentage_level(desc, pct) + return -1, "", GREEN, "", False + + +def _format_supply_bar(pct: int) -> str: + """Build a progress bar string for a supply percentage.""" + if pct < 0: + return "" + filled = pct * PROGRESS_BAR_WIDTH // 100 + empty = PROGRESS_BAR_WIDTH - filled + return f"[{'█' * filled}{'░' * empty}]" + + +def _process_supply_item(desc: str, max_val: int, level: int) -> SupplyStatus: + """Process a single supply item into display info.""" + pct, status_text, color, warning, needs_replacement = _classify_supply_level( + desc, max_val, level + ) + bar_text = _format_supply_bar(pct) + return SupplyStatus(color, bar_text, status_text, warning, needs_replacement) + + +def _parse_supply_value(values: list[str], index: int) -> int: + """Safely parse an integer from a supply value list.""" + try: + return int(values[index]) + except (IndexError, ValueError): + return 0 + + +def _collect_supply_items( + result: NetworkResult, +) -> tuple[list[SupplyStatus], list[str]]: + """Parse and collect supply items with their descriptions.""" + items: list[SupplyStatus] = [] + descs: list[str] = [] + for i, desc in enumerate(result.supplies.descriptions): + max_val = _parse_supply_value(result.supplies.max_values, i) + level = _parse_supply_value(result.supplies.levels, i) + items.append(_process_supply_item(desc, max_val, level)) + descs.append(desc) + return items, descs + + +def render_life_bar( + label: str, + pct: int, + *, + exhausted: bool = False, + low: bool = False, + exhausted_note: str = "", + low_note: str = "", +) -> None: + """Print one consumable's remaining-life bar. + + The toner and drum bars in the page-count estimate were byte-for-byte the + same logic with different colours and notes, which is what this collapses. + + Args: + label: Displayed name, already padded by the caller if needed. + pct: Percentage remaining, 0-100. + exhausted: Render red with ``exhausted_note``. + low: Render yellow with ``low_note``. Ignored when *exhausted*. + exhausted_note: Suffix shown when *exhausted*. + low_note: Suffix shown when *low*. + """ + filled = pct * PROGRESS_BAR_WIDTH // 100 + meter = f"[{'█' * filled}{'░' * (PROGRESS_BAR_WIDTH - filled)}]" + if exhausted: + color, note = RED, exhausted_note + elif low: + color, note = YELLOW, low_note + else: + color, note = GREEN, "" + _out(f" {BOLD}{label}{RESET} {color}{meter} ~{pct}%{note}{RESET}") diff --git a/python_pkg/brother_printer/display.py b/python_pkg/brother_printer/display.py index bfdd3319..8bb3b7fc 100644 --- a/python_pkg/brother_printer/display.py +++ b/python_pkg/brother_printer/display.py @@ -3,20 +3,34 @@ from __future__ import annotations import sys +from typing import TYPE_CHECKING +# ── Shared display helpers ─────────────────────────────────────────── +# Re-exported through __all__: tests/test_display.py imports these names from +# this module. Only the un-patched supply *maths* moved to _supply -- everything +# the tests reach with mock.patch has to stay resolvable here. +from python_pkg.brother_printer._severity import ( + _SEVERITY_COLORS, + _SEVERITY_ICONS, + _SEVERITY_SUMMARIES, +) +from python_pkg.brother_printer._supply import ( + _classify_percentage_level, + _classify_supply_level, + _collect_supply_items, + _format_supply_bar, + _parse_supply_value, + _process_supply_item, + render_life_bar, +) from python_pkg.brother_printer.constants import ( BOLD, CYAN, DIM, DRUM_RATED_PAGES, GREEN, - PROGRESS_BAR_WIDTH, RED, RESET, - SNMP_LEVEL_LOW, - SNMP_LEVEL_OK, - SUPPLY_LOW_PCT, - SUPPLY_WARN_PCT, TONER_RATED_PAGES, YELLOW, _out, @@ -30,13 +44,23 @@ from python_pkg.brother_printer.cups_queue import ( display_cups_queue_status, get_cups_queue_status, ) -from python_pkg.brother_printer.data_classes import ( - NetworkResult, - SupplyStatus, - USBResult, -) -# ── Shared display helpers ─────────────────────────────────────────── +if TYPE_CHECKING: + from python_pkg.brother_printer.data_classes import ( + NetworkResult, + USBResult, + ) + +__all__ = [ + "_classify_percentage_level", + "_classify_supply_level", + "_collect_supply_items", + "_format_supply_bar", + "_parse_supply_value", + "_process_supply_item", + "display_network_results", + "display_usb_results", +] def _display_report_header() -> None: @@ -101,36 +125,20 @@ def _display_page_count_estimate(printer_total: int = 0) -> None: f" and reads high.{RESET}" ) _out() - # Toner bar - toner_pct = estimate.toner_pct_remaining - toner_filled = toner_pct * PROGRESS_BAR_WIDTH // 100 - toner_empty = PROGRESS_BAR_WIDTH - toner_filled - toner_bar = f"[{'█' * toner_filled}{'░' * toner_empty}]" - if estimate.toner_exhausted: - toner_color = RED - toner_note = " ← REPLACE NOW" - elif estimate.toner_low: - toner_color = YELLOW - toner_note = " ← order soon" - else: - toner_color = GREEN - toner_note = "" - _out( - f" {BOLD}Toner:{RESET} {toner_color}{toner_bar} ~{toner_pct}%" - f"{toner_note}{RESET}" + render_life_bar( + "Toner:", + estimate.toner_pct_remaining, + exhausted=estimate.toner_exhausted, + low=estimate.toner_low, + exhausted_note=" ← REPLACE NOW", + low_note=" ← order soon", + ) + render_life_bar( + "Drum: ", + estimate.drum_pct_remaining, + low=estimate.drum_near_end, + low_note=" ← nearing end", ) - # Drum bar - drum_pct = estimate.drum_pct_remaining - drum_filled = drum_pct * PROGRESS_BAR_WIDTH // 100 - drum_empty = PROGRESS_BAR_WIDTH - drum_filled - drum_bar = f"[{'█' * drum_filled}{'░' * drum_empty}]" - if estimate.drum_near_end: - drum_color = YELLOW - drum_note = " ← nearing end" - else: - drum_color = GREEN - drum_note = "" - _out(f" {BOLD}Drum:{RESET} {drum_color}{drum_bar} ~{drum_pct}%{drum_note}{RESET}") _out( f" {DIM}Based on pages since last replacement" f" vs rated capacity (toner ~{TONER_RATED_PAGES}," @@ -183,34 +191,6 @@ def _display_usb_device_info(result: USBResult) -> None: _out(f"{BOLD}Toner Save:{RESET} OFF") -_SEVERITY_ICONS: dict[str, str] = { - "ok": "✓", - "info": "i", - "warn": "⚡", - "critical": "⚠", -} -_SEVERITY_COLORS: dict[str, str] = { - "ok": GREEN, - "info": CYAN, - "warn": YELLOW, - "critical": RED, -} -_SEVERITY_SUMMARIES: dict[str, str] = { - "ok": f"{GREEN}{BOLD}✓ Printer is healthy. No replacements needed.{RESET}", - "info": ( - f"{CYAN}{BOLD}i Printer is busy/processing. No replacements needed.{RESET}" - ), - "warn": ( - f"{YELLOW}{BOLD}⚡ WARNING: Maintenance will be needed" - f" soon.{RESET}\n{YELLOW} Order replacement parts" - f" now to avoid interruption.{RESET}" - ), - "critical": ( - f"{RED}{BOLD}⚠ ACTION REQUIRED: Replacement or fix needed now!{RESET}" - ), -} - - def _format_status_detail( severity: str, short_text: str, action: str, result: USBResult ) -> None: @@ -291,49 +271,6 @@ def display_usb_results(result: USBResult) -> None: # ── Network supply level helpers ───────────────────────────────────── -def _classify_percentage_level(desc: str, pct: int) -> tuple[int, str, str, str, bool]: - """Classify a supply by its calculated percentage.""" - if pct <= SUPPLY_LOW_PCT: - return pct, f"{pct}%", RED, f"{desc} at {pct}%.", True - if pct <= SUPPLY_WARN_PCT: - return pct, f"{pct}%", YELLOW, f"{desc} at {pct}% -- order soon.", False - return pct, f"{pct}%", GREEN, "", False - - -def _classify_supply_level( - desc: str, max_val: int, level: int -) -> tuple[int, str, str, str, bool]: - """Classify a supply level. Returns (pct, status, color, warning, replace).""" - if level == SNMP_LEVEL_OK: - return -1, "OK", GREEN, "", False - if level == SNMP_LEVEL_LOW: - return -1, "LOW", RED, f"{desc} is LOW.", True - if level == 0: - return 0, "EMPTY", RED, f"{desc} is EMPTY -- replace now!", True - if max_val > 0: - pct = min(level * 100 // max_val, 100) - return _classify_percentage_level(desc, pct) - return -1, "", GREEN, "", False - - -def _format_supply_bar(pct: int) -> str: - """Build a progress bar string for a supply percentage.""" - if pct < 0: - return "" - filled = pct * PROGRESS_BAR_WIDTH // 100 - empty = PROGRESS_BAR_WIDTH - filled - return f"[{'█' * filled}{'░' * empty}]" - - -def _process_supply_item(desc: str, max_val: int, level: int) -> SupplyStatus: - """Process a single supply item into display info.""" - pct, status_text, color, warning, needs_replacement = _classify_supply_level( - desc, max_val, level - ) - bar_text = _format_supply_bar(pct) - return SupplyStatus(color, bar_text, status_text, warning, needs_replacement) - - def _display_supply_warnings(*, needs_replacement: bool, warnings: list[str]) -> None: """Display supply level warnings summary.""" _out() @@ -349,28 +286,6 @@ def _display_supply_warnings(*, needs_replacement: bool, warnings: list[str]) -> _out(f"{GREEN}{BOLD}✓ All consumables are at healthy levels.{RESET}") -def _parse_supply_value(values: list[str], index: int) -> int: - """Safely parse an integer from a supply value list.""" - try: - return int(values[index]) - except (IndexError, ValueError): - return 0 - - -def _collect_supply_items( - result: NetworkResult, -) -> tuple[list[SupplyStatus], list[str]]: - """Parse and collect supply items with their descriptions.""" - items: list[SupplyStatus] = [] - descs: list[str] = [] - for i, desc in enumerate(result.supplies.descriptions): - max_val = _parse_supply_value(result.supplies.max_values, i) - level = _parse_supply_value(result.supplies.levels, i) - items.append(_process_supply_item(desc, max_val, level)) - descs.append(desc) - return items, descs - - def _display_supply_levels(result: NetworkResult) -> None: """Display consumable supply levels section.""" _out()