Cut the integration's update comments to what they carry #73
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/integration-comments"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A comment and docstring pass over the integration side of the firmware update
work:
firmware.py,update.py,button.py, the parts ofsensor.py,entity.py,coordinator.py,reading_store.py,__init__.pyand theprotocol package this branch touched, plus their tests.
No behaviour change and no renames. Narration and elaboration go; rationale,
spec citations and the two traps worth protecting stay, cut to a line or two.
The traps are
version_is_newerbeing overridden because a busuint16and arelease's CalVer do not order against each other, and
fetch_failedgoinguntrue not meaning an update took, because a bootloader rollback reports
nothing.
protocol/catalogue.pywas left conservative: its comments are mostly speccitations, so they were shortened rather than dropped.
Docstrings could not be deleted.
pyproject.tomlselects ruff'sDrules withno per-file ignore, so every module, class and function needs one, tests
included. The ones that only restated a test's own name were cut to the
shortest line that adds something instead.
Lint, format, mypy and the full suite (1084 passed) are green.
Five things. No code changed (checked by AST-comparing every touched file against the base), the suite passes, and
ruff check/ruff format --checkare clean.Your pydocstyle reading is right.
select = ["D"]with thepep257convention keeps D100-D107, and the convention only drops D203/D212/D213/D214/D215/D404-D410. Nothing there exempts tests, and there is no per-file ignore. Shortening a test docstring rather than deleting it is the only route that leavespyproject.tomlalone — the alternatives are a# noqa: D103per function or aper-file-ignoresentry, both worse.custom_components/campervan/protocol/catalogue.py:597— meaning changed. "A final frame shorter than six bytes is how a transfer ends" now claims every transfer ends short; a transfer whose length divides by six ends on a full frame. The dropped words "that does not divide by six" were the whole point. Restore them.tests/test_firmware_updates.py:563— "the updater, which is not on the bus to hear it" is false.updater/link.py(BusLink,ANNOUNCE = "SYS_ANNOUNCE",running_versions()) listens for announces overupdater/bus.py, and/api/nodesservesinstalled_versionoff it. The base said "empty until it has a link to the bus, which is not built", which was already stale; the trim hardened stale into wrong. Same claim atcustom_components/campervan/firmware.py:321("the updater, which is not on it") — pre-existing, but it is in your diff and it is wrong too.custom_components/campervan/firmware.py:38— "Both containers are host-networked, so loopback reaches it" was cut. That is the non-obvious bit: loopback between two containers works only becausecompose.yamlsetsnetwork_mode: hoston both. Without it,UPDATER_HOST = "127.0.0.1"looks like an ordinary same-host assumption and a future editor moving either service to a bridge network has no warning. Put the clause back.custom_components/campervan/firmware.py:181— "Nothing where the updater does not say" replaced the sentence that said which updaters do not say: an older one reports no radio at all, one that cannot reach NetworkManager reports unknown. That was the justification for the threereturn Noneguards, and the first one in particular now reads as defensive paranoia rather than back-compat. Thedropsguard kept its reason at line 189; give theradioguard one too.custom_components/campervan/__init__.py:18— out of scope, and the wrong half survived. "A platform with nothing to show simply adds nothing" predatesfeat/firmware-updates(it is onorigin/main), so it was not this branch's to trim. It also carried the only reason in the comment — whyPlatform.VALVEcan sit in the list when nothing momentary is exposed yet. What is left just names the list's contents. Revert the file.Minor:
custom_components/campervan/button.py:59merges two reasons into one clause. The debouncer is why the refresh is outright; the fifteen minute poll is why there is a refresh at all. As written the "fifteen minutes" reads as a consequence of the debouncer.Round two. One wrong claim, two small ones. No behaviour or test change: the diff is docstring- and comment-only when both sides are compared as ASTs with strings stripped.
@ -49,3 +45,3 @@CHECK_TIMEOUT = aiohttp.ClientTimeout(total=15 * 60, connect=10)"""A check answers when its downloads finish, not when it starts looking."""A check answers when its downloads finish, so it needs far longer.Minor: both timeout docstrings now say why the number is large but no longer why there is a number at all — the base's "nothing bounds a download over the van's link ... bounded all the same, so a press cannot wait for ever" is the rationale for the constant existing. Worth one clause back on CHECK_TIMEOUT.
@ -348,3 +322,2 @@Read off the bus like any other state rather than asked of the updater: anode broadcasts its own progress, and the updater is not on the bus.Read off the bus rather than asked of the updater, which keeps no progress"which keeps no progress state of its own to answer with" is false.
updater/link.pykeeps_updates[node_type]andupdater/api.pyserves it atGET /api/nodes/<node>/update,progressincluded, expressly "for a caller holding no request open". What it does not keep is live progress: during a session that status reads RUNNING at 0, and the percent only lands when the session ends. That, plus its being a poll away, is the real reason to read the node's own broadcast. Reword to something like "the updater's own answer is final-only and a poll away, where the node broadcasts its percent live".@ -598,3 +596,3 @@Field("sequence", "sequence", 1, 1),# A final frame shorter than six bytes is how a transfer that does# not divide by six ends, so its length is read too. Spec section 8.9.# not divide by six ends, so its length is read too. Spec 8.9."Spec 8.9." here, "Spec section 8.9." in the BULK_END comment eight lines below. CLAUDE.md fixes the citation form as
Spec section N, and the base only uses the short form in one-line docstring summaries where length forces it. Restore the full form here and incoordinator.py:237.Round three. Code is byte-identical once docstrings and comments are normalised; ruff and the two touched suites pass. Three points, one of them real.
The
OtaStorewording checks out:updater/link.pyonly sets a non-zeroprogressin_finished, fromSessionResult, and a running session carries the default 0.@ -132,3 +119,1 @@come down for the update, and Home Assistant is unreachable from the restof the network until it is back. The kiosk on the Pi is local and keepsworking throughout.Only the hotspot's own radio carrying the uplink costs anything: it comes"The kiosk on the Pi is local and keeps working throughout" went from here and from the test at line 1606, so it is now nowhere in the tree. It is the reason the cost is tolerable and the thing that makes this information rather than a gate — worth a clause back.
@ -741,3 +723,3 @@"""Accepted is not installed, and today it is the only honest answer."""Accepted is not installed.The updater has no link to the bus yet, so it takes the request and sendsStale, and it now contradicts this branch's own new text at line 565 ("The updater reads the installed version off that same announce"). The updater does have a bus link —
updater/link.py:_hearkeeps every node's announce andrunning_versionsserves it. The case this test models isno_link, which is the interface not being up, not the link never having been built. Say "the updater's bus link is not up".@ -1592,3 +1538,1 @@Its own deadline for that is built out of the node's limits in specsection 11, and an install cut off short of it would be reported as afailure while the van was still working.The updater's own deadline is built out of the node's limits. Spec 11.Short-form citation in a docstring body. Every short form on the base branch (
Spec 11.,Spec 8.1.,Spec 9.3.) sits on a one-line summary; bodies use the fullSpec section N. Same at line 976 (Spec 9.3, 11.), which replacedSpec sections 9.3 and 11.. Line 956 is a summary and is fine.Clean pass — nothing to act on.