rob/CampervanHomeAssistant
Watch
1
0
Fork
You've already forked CampervanHomeAssistant
 0
Code Issues Pull requests Projects Releases 26 Packages Wiki Activity Actions

Fix six spots where a comment was standing in for clearer code #106

Merged
Claude merged 6 commits from chore/fix-comment-smells into refactor/code-hygiene 2026-09-28 07:50:42 +00:00
Conversation 1 Commits 6 Files changed 5 +34 -35
Claude commented 2026-09-28 07:48:45 +00:00
Collaborator

Six small fixes, one commit each, all behaviour-preserving:

  • CampervanNodeEntity.__init__ sets node and device info directly instead of routing through async_redescribed's getattr guard.
  • UpdaterClient._request's patience parameter is renamed client_timeout.
  • discovery.py's _ask and _judge_what_arrived share a new _finish() for the clear-then-notify step.
  • traffic.py's async_handle_frame uses readings.routed() instead of re-implementing the reserved-source check.
  • traffic.py's BUCKETS becomes CLOSED_BUCKETS, dropping the +1 docstring.
  • platforms.py's two follow_the_inventory closures share a new ready_to_name() predicate.

Each commit deletes the comment that was explaining the awkwardness it fixes. tools/check passes; existing tests are the gate for behaviour.

Six small fixes, one commit each, all behaviour-preserving: - `CampervanNodeEntity.__init__` sets `node` and device info directly instead of routing through `async_redescribed`'s `getattr` guard. - `UpdaterClient._request`'s `patience` parameter is renamed `client_timeout`. - `discovery.py`'s `_ask` and `_judge_what_arrived` share a new `_finish()` for the clear-then-notify step. - `traffic.py`'s `async_handle_frame` uses `readings.routed()` instead of re-implementing the reserved-source check. - `traffic.py`'s `BUCKETS` becomes `CLOSED_BUCKETS`, dropping the `+1` docstring. - `platforms.py`'s two `follow_the_inventory` closures share a new `ready_to_name()` predicate. Each commit deletes the comment that was explaining the awkwardness it fixes. `tools/check` passes; existing tests are the gate for behaviour.
Extract ready_to_name for the two follow_the_inventory closures
All checks were successful
Lint, type check and test / hassfest (pull_request) Successful in 59s
Details
Lint, type check and test / quality (pull_request) Successful in 2m20s
Details
Lint, type check and test / release (pull_request) Has been skipped
Details
e1a2ffed49
Claude merged commit e2c2f0ad6a into refactor/code-hygiene 2026-09-28 07:50:42 +00:00
Claude deleted branch chore/fix-comment-smells 2026-09-28 07:50:42 +00:00
Claude reviewed 2026-09-28 07:52:21 +00:00
Claude left a comment

Checked all six commits against the pre-refactor behaviour. No bugs found.

Specifically on discovery.py (commit 3): the _finish() extraction is safe. _ask()'s new call to _finish() now clears self._wait = None, which the old code never did there — but _ask()'s task is only ever created by async_scan(), which calls _stop_waiting() (and hence sets _wait = None) immediately before creating it, and waiting/scanning guards keep _wait_for_the_rest() from setting a new one while _ask() runs. So _wait is always already None for the whole lifetime of _ask(), and adding the clear there is a genuine no-op. The short branch in _judge_what_arrived correctly keeps its inline clear-without-notify instead of routing through _finish(), avoiding the spurious settled=True blip you flagged — confirmed against 706e7bb^.

Two purely cosmetic, non-blocking observations:

  • traffic.py async_handle_frame: when routed() returns None (reserved/broadcast source), the code now decodes message.arbitration_id a second time to get identifier for the failure-path log line, where the old code decoded once up front. Harmless (rare path), just a duplicate call — not worth blocking on.
  • discovery.py _finish()'s merged comment drops the old "whatever could not be read has just been asked about" rationale for clearing _lost_a_frame. Reasonable to drop since it only made sense in _ask()'s context (a fresh query was just sent) and would have been misleading if kept verbatim for the _judge_what_arrived caller — no action needed, just flagging that the specific "why" for that field is no longer spelled out anywhere.

Everything else — entity.py's direct __init__ assignment, firmware.py's client_timeout rename, traffic.py's CLOSED_BUCKETS arithmetic (3 closed + 1 in-flight = same maxlen=4 as before), and platforms.py's ready_to_name predicate — is behaviourally equivalent to what it replaced, and the spec citation removed from async_setup_node_entities's comment (section 8.1) is still carried by the module docstring at the top of platforms.py.

Checked all six commits against the pre-refactor behaviour. No bugs found. Specifically on discovery.py (commit 3): the `_finish()` extraction is safe. `_ask()`'s new call to `_finish()` now clears `self._wait = None`, which the old code never did there — but `_ask()`'s task is only ever created by `async_scan()`, which calls `_stop_waiting()` (and hence sets `_wait = None`) immediately before creating it, and `waiting`/`scanning` guards keep `_wait_for_the_rest()` from setting a new one while `_ask()` runs. So `_wait` is always already `None` for the whole lifetime of `_ask()`, and adding the clear there is a genuine no-op. The `short` branch in `_judge_what_arrived` correctly keeps its inline clear-without-notify instead of routing through `_finish()`, avoiding the spurious `settled=True` blip you flagged — confirmed against `706e7bb^`. Two purely cosmetic, non-blocking observations: - traffic.py `async_handle_frame`: when `routed()` returns `None` (reserved/broadcast source), the code now decodes `message.arbitration_id` a second time to get `identifier` for the failure-path log line, where the old code decoded once up front. Harmless (rare path), just a duplicate call — not worth blocking on. - discovery.py `_finish()`'s merged comment drops the old "whatever could not be read has just been asked about" rationale for clearing `_lost_a_frame`. Reasonable to drop since it only made sense in `_ask()`'s context (a fresh query was just sent) and would have been misleading if kept verbatim for the `_judge_what_arrived` caller — no action needed, just flagging that the specific "why" for that field is no longer spelled out anywhere. Everything else — `entity.py`'s direct `__init__` assignment, `firmware.py`'s `client_timeout` rename, `traffic.py`'s `CLOSED_BUCKETS` arithmetic (`3` closed + 1 in-flight = same `maxlen=4` as before), and `platforms.py`'s `ready_to_name` predicate — is behaviourally equivalent to what it replaced, and the spec citation removed from `async_setup_node_entities`'s comment (section 8.1) is still carried by the module docstring at the top of platforms.py.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
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!106
No description provided.