Prune comments and docstrings that only restate the code in updater/ #108
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/prune-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?
Delete-only pass over
updater/*.py: removed comments and docstrings thatonly restated a name, signature, type or the next line, per the enumerated
list of candidates. Trimmed a handful of multi-line comments that argued
rather than explained down to their one load-bearing sentence.
Kept everything citing a spec section on a constant or enum member, the
session timeout constants, the security-rationale comments in
firmware.py,and the idempotency/warning comments called out for keeping.
No code changes —
tools/check(ruff lint, ruff format, mypy, pytest) isgreen.
a4ef7b1d2204b226dd52Verified mechanically: stripped every docstring/bare string-expression from both
refactor/code-hygiene's and this branch'supdater/*.pyand diffed the resulting ASTs — identical for all 9 touched files. So this is genuinely delete/reword-only; no logic changed anywhere.tools/checkis fully green (ruff lint, ruff format --check, mypy, pytest, 1223 passed). The three barepassbodies (RadioError,FetchStoppedError,_ShortDownloadError) are the expected mechanical fallout of removing each class's only content. Merge base is the current tip ofrefactor/code-hygiene(8ebfe26) — no rebase artifacts. All the explicitly protected items I sampled (session timeout constants,FRAME_GAP,MAX_IMAGE_PATH/MAX_MANIFEST_BYTES,TokenSafeRedirects,ForgejoReleases.request's rationale,Radio.restore's idempotency docstring,run_nmcli's raises note,open_socket's tests-replace-it note, theNEVER:comment in link.py,_restore_on_deadline's cleared-not-disarmed comment, the autoconnect-no warning) are all untouched.Two spots where trimming went past "restates the code" into dropping real content:
updater/session.pymodule docstring: the deleted middle paragraph ("A node pulls its own firmware, so all this does is tell it where from... No image travels the bus.") is architecturally significant and isn't obvious from reading the module — it's the reason the session only ever sends credentials/pointers over the bus. Worth keeping, at least as one sentence. The "Spec section 11" citation on the docstring was also dropped in the reword (spec citations elsewhere in the file survive, but this was the module-level signpost).updater/session.py_watch's docstring lost its rationale, not just an example: the original explained why an announce can arrive before flashing starts (it's also how a node answersQRY_DESCRIPTORS) and what breaks if you don't guard for it (a mid-download announce would read as success on the old firmware and pull the hotspot down on a node still writing). What's left ("Count an announce only once the node has been seen writing an image") states the what but not the trap a future editor would hit changing thenot flashingguard. This reads like exactly the kind of "warning about a genuine trap" comment the project's conventions call out for keeping, even though it wasn't on the explicit protected list.Neither blocks the PR — both are one sentence to restore if you want the rationale back on record. Nothing else in the diff looks over-pruned; the rest of the trims (e.g.
Outcome/UP_TO_DATEenum docstrings, the_UPLOADEDglobal's docstring, api.py's merged lock comment) read as legitimate restatement/argument trims that keep the load-bearing sentence.Thanks for the review. Both flagged items were explicit trims called out in the task brief rather than my own judgement calls:
session.pymodule docstring was directed to be trimmed to "its second paragraph (the one about BULK_END being the only reply)" — dropping the "a node pulls its own firmware" paragraph and the top-line spec citation was the instructed outcome, not an oversight._watch's docstring was directed to keep "the one sentence about an announce only counting once flashing was seen" — the rationale sentences were explicitly meant to go, leaving only that one load-bearing sentence.Leaving both as instructed rather than reverting. No other changes needed from this round.
Re-reviewed at the same commit (
04b226d, no changes since the last round).Checked your reply against the diff: both trims match what you describe. The
session.pymodule docstring now keeps exactly theBULK_END-is-the-only-reply paragraph (promoted to the summary line), and_watch's docstring keeps exactly the "count an announce only once flashing was seen" clause, dropping theQRY_DESCRIPTORS/mid-download rationale as directed. Since both were explicit, directed outcomes rather than judgement calls the agent made on its own, I'm not re-raising them — the prior round's observations stand as read-if-you-want-the-rationale-back notes, not blockers.Fresh pass over the rest of the diff found nothing further: the
_UPLOADEDdocstring trim looked worth a second look (it drops "never touchesimage.json"), but that fact is preserved next toCachedImage.version's field comment, so nothing is actually lost. Everything on the explicitly-protected list (session timeout constants,FRAME_GAP,MAX_IMAGE_PATH/MAX_MANIFEST_BYTES,TokenSafeRedirects,ForgejoReleases.request,Radio.restore,run_nmcli,open_socket, link.py'sNEVER:comment,_restore_on_deadline's comment, radio.py's autoconnect-no warning, the logged-not-raised comments) is untouched.tools/checkis green: ruff lint, ruff format --check, mypy, and pytest (1223 passed, 3 skipped).Clean to merge.