Cut the updater's comments to the reasons they carry #70
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/updater-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 pass over the updater service, its compose and image definitions, and
its two test modules. No behaviour change: the diff is comments and docstring
prose only, and the suite passes unchanged.
Comment lines across those files go from 272 to 196.
#comments drop from 50to 33; the rest is docstring prose cut back to the reason it carries.
The load-bearing reasons in
radio.pyare all kept, just shorter: whyconnection downblocks autoconnect and what the state file is for, why theprofile must never be paired with
autoconnect no, why the hotspot address isread back rather than assumed, why the deadline clears itself rather than
disarming, and why the restore holds a lock.
Docstrings could not be deleted outright. Ruff runs
Dwith the pep257convention and no per-file ignores, so D100 to D107 require one on every module,
class, function and test. They are shortened instead. Lifting that for
tests/would need
pyproject.toml, which is outside this change.No behaviour change: the parsed ASTs of every touched Python file match the base once docstrings are stripped, and the non-comment lines of
compose.yamland theDockerfileare byte-identical. Lint, format and the updater tests are green.Findings inline. The one that matters is the lost
try/finallyreason inRadio.restore.@ -39,3 +37,1 @@# No privileged and no cap_add, unlike the entry above: NetworkManager does# the privileged work in its own process and this only sends it D-Bus# messages, and a CAN_RAW socket on an interface already up needs neither.# No privileged and no cap_add, unlike above: NetworkManager does theDropped "and a CAN_RAW socket on an interface already up needs neither." That was the second half of the answer, and the one the reader needs given
network_mode: host # can0three lines down — the D-Bus half says nothing about why CAN needs no caps.@ -92,3 +89,2 @@# Raising it with no handler installed would kill the test run outright,# taking the report with it.# With no handler installed this would kill the test run outright.Misleading after compression. "this" now sits directly above the
assert, which reads as though the assert kills the test run; the thing that would israise_signaltwo lines down. Name it: "#raise_signalbelow would kill the test run outright with no handler installed."@ -8,3 +3,1 @@The failure that matters is not a failed update but a Pi left as an access pointwith no route home, so every path out of here restores: the deadline, thefailures while raising it, and the shutdown the service arranges.NetworkManager owns `wlan0` on the host, so this drives it through `nmcli` overThe module docstring lost "Nodes pull firmware over WiFi, so the Pi carries an access point for as long as an update runs." That is the premise for the whole file and it is not stated anywhere else in
updater/—__init__.pyjust says "the Pi's radio". The new opening starts at NetworkManager mechanics with no answer to why an access point exists at all.@ -300,0 +260,4 @@Idempotent, because a finished update, a failure, the deadline and ashutdown can all arrive at it. The lock is what makes that true: two atonce would each take the other's work as still to do. The uplink is onlyforgotten once it is back up, so a later attempt still knows what it owes.Load-bearing reason dropped: "The uplink goes back up even if taking the hotspot down failed." That sentence is the only thing explaining the
try/finallytwo lines below. Without it a future editor reads thefinallyas stylistic and flattens it, and a failed_remove_hotspotthen leaves the Pi with no uplink — the exact failure this module exists to prevent.Also, "Idempotent, because a finished update, a failure, the deadline and a shutdown can all arrive at it" inverts the causation: those callers are why it has to be idempotent, not why it is. And the lock makes concurrent calls safe, not idempotent — two different properties now welded into one sentence.
@ -71,3 +67,2 @@stop.set()# A fetch runs in a thread that cancelling the task waiting on it does# not reach, so it is told to give up before anything is cancelled.# Cancelling the waiting task does not reach the fetch's own thread.The surviving half explains why
FETCH_STOPexists but not why this line comes before the cancel loop, which was the ordering constraint the original carried ("told to give up before anything is cancelled"). Something like "...so it is told to give up before the cancels below" puts it back in the same number of words.All five taken, all five accurate. Nothing further.
restore's docstring now matches thetry/finallyand the lock is credited only with the concurrency.Before the cancels:carries the ordering in fewer words than the original did. Still comment-only againstfeat/firmware-updates— ASTs match with docstrings stripped, non-comment lines ofcompose.yamland theDockerfileunchanged, andconftest.py,fakes.pyandpyproject.tomluntouched.One thing worth knowing rather than acting on: the compose comment swapped the D-Bus half out for the CAN_RAW half rather than keeping both, so "this only sends it D-Bus messages" is gone. That reason still lives in the Dockerfile next to the
network-managerinstall, so it is not lost — leave it.Review came back clean on the second round.
feat/firmware-updatesmoved under this branch while it was open (#72 merged), so it is merged in at055a22a; no overlap with the files here.CI is green on the merged head: run #1458 on
055a22a, success.Final count across the nine files: 272 comment lines to 199.
#comments 50 to 33, docstring prose 222 lines to 166.Not merging — left open as asked.