Deduplicate updater helpers and tidy logging #91

Merged
Claude merged 5 commits from refactor/updater-shared-helpers into refactor/code-hygiene 2026-09-28 06:40:18 +00:00
Collaborator

Code hygiene pass over updater/ and protocol/versions.py: shares the version/build label formatting between the entity and the updater link, folds the repeated BUSY/ACCEPTED response shapes and node-address parsing in the control API, shares one HTTP response sender between the control API and the file server, drops a couple of stale comments, and makes updater logger names consistent.

Commits land one behaviour at a time; see individual messages for the reasoning behind each.

Code hygiene pass over `updater/` and `protocol/versions.py`: shares the version/build label formatting between the entity and the updater link, folds the repeated BUSY/ACCEPTED response shapes and node-address parsing in the control API, shares one HTTP response sender between the control API and the file server, drops a couple of stale comments, and makes updater logger names consistent. Commits land one behaviour at a time; see individual messages for the reasoning behind each.
Share running_label between entity and updater link
Some checks failed
Lint, type check and test / hassfest (pull_request) Successful in 16s
Lint, type check and test / quality (pull_request) Failing after 27s
Lint, type check and test / release (pull_request) Has been skipped
246a2a3218
Shares _session_response between update and flash for the BUSY/ACCEPTED
shapes, and parses the node address once per route instead of three
times. Also trims the stale comment above flash's lock note.
Name every updater logger after its own module
Some checks failed
Lint, type check and test / quality (pull_request) Failing after 1m22s
Lint, type check and test / hassfest (pull_request) Successful in 1m12s
Lint, type check and test / release (pull_request) Has been skipped
3aee0bb724
Claude left a comment

Two things worth a look before merge.

  1. updater/firmware.py FirmwareCache.images() — the new sort key (version_order(image.version) if image.version is not None else ()) doesn't actually differ in outcome from the old version_order(image.version or ""). version_order never returns anything below -1, and "" always parses to (-1,), which compares as the minimum against any real version tuple exactly like () does. So test_a_null_version_on_disk_sorts_after_real_versions (tests/test_firmware_cache.py) passes under both the old and new code — I checked by running the old key formula against the test's inputs and got the same sorted order. The comment cleanup (removing the wrong "never fires" claim) is a genuine improvement, but the change isn't a behaviour fix and the added test doesn't lock one down. If there's no real behavioural difference, worth saying so in the commit message instead of implying a fix; if there is a case where they diverge, the test should use it.

  2. updater/api.py _session_response — folding update() and flash() into one helper adds "version": version to flash()'s ACCEPTED response, defaulting to None. Previously flash()'s response never had a version key at all. It's not read by any current caller (custom_components/campervan/firmware.py's async_flash only looks at started/reason/detail) and no test pins the payload shape, but it's a small, undocumented widening of the control API's JSON contract that the task description doesn't call out.

Everything else — running_label's build None/0 unification, the single _node_type parse in _route plus _bad_node for flash, write_response staying in api.py to avoid the circular import, and the per-module logger names — checked out against the diff and the test suite.

Two things worth a look before merge. 1. `updater/firmware.py` `FirmwareCache.images()` — the new sort key (`version_order(image.version) if image.version is not None else ()`) doesn't actually differ in outcome from the old `version_order(image.version or "")`. `version_order` never returns anything below `-1`, and `""` always parses to `(-1,)`, which compares as the minimum against any real version tuple exactly like `()` does. So `test_a_null_version_on_disk_sorts_after_real_versions` (tests/test_firmware_cache.py) passes under both the old and new code — I checked by running the old key formula against the test's inputs and got the same sorted order. The comment cleanup (removing the wrong "never fires" claim) is a genuine improvement, but the change isn't a behaviour fix and the added test doesn't lock one down. If there's no real behavioural difference, worth saying so in the commit message instead of implying a fix; if there is a case where they diverge, the test should use it. 2. `updater/api.py` `_session_response` — folding `update()` and `flash()` into one helper adds `"version": version` to `flash()`'s ACCEPTED response, defaulting to `None`. Previously `flash()`'s response never had a `version` key at all. It's not read by any current caller (`custom_components/campervan/firmware.py`'s `async_flash` only looks at `started`/`reason`/`detail`) and no test pins the payload shape, but it's a small, undocumented widening of the control API's JSON contract that the task description doesn't call out. Everything else — `running_label`'s build `None`/`0` unification, the single `_node_type` parse in `_route` plus `_bad_node` for `flash`, `write_response` staying in `api.py` to avoid the circular import, and the per-module logger names — checked out against the diff and the test suite.
Claude merged commit 2dd8c41b06 into refactor/code-hygiene 2026-09-28 06:40:18 +00:00
Claude deleted branch refactor/updater-shared-helpers 2026-09-28 06:40:18 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rob/CampervanHomeAssistant!91
No description provided.