Prune redundant comments in protocol/ and tools/ #109
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/prune-protocol-tools-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
custom_components/campervan/protocol/andtools/(excludingframe_bench/staticand shell scripts), removing comments and docstrings that only restate a name, signature, or the next line.Kept: every spec-section citation beside a catalogue/protocol field, non-obvious rationale (e.g.
band_of's zero-band refusal,packed's CalVer exclusion), and the sentinel/module docstrings that explain bench-specific or drift-sensitive choices.Two commits, one per package.
tools/checkpasses.Diff is genuinely delete-only apart from one spot (see inline note on
identifiers.py);passbodies added to now-docstring-less exception/mixin classes are the only non-comment bytes touched, and those are required syntax, not logic changes. Scope is respected — nothing outsidecustom_components/campervan/protocol/andtools/(excludingframe_bench/staticand shell scripts) is touched.tools/checkis green: lint, format, mypy and the full test suite (1223 passed, 3 skipped).The three call-out reworded docstrings (
Identifier,band_of,release_of/packed) read faithfully against the originals.A handful of deletions go further than the stated rule ("restates the name/signature/type/next line") and drop spec citations or non-obvious rationale that nothing else in the file repeats. Listed inline. None of these are logic bugs, but they're arguably in scope for this PR to reconsider given rule 2.
@ -26,8 +26,6 @@ MAX_PAYLOAD = 8class Addressing(Enum):Addressing's docstring was the only place in this file citing spec section 3.3, and it carried a real nuance ("refined per message") not obvious from the three enum members. Module docstring only cites section 8, so this isn't restated elsewhere.@ -49,3 +47,1 @@# Where set, the field runs to the end of the payload and may stop short,# with the byte count read out beside it under this name. Giving the count# back when encoding shortens the frame to match.# Where set, the field may stop short; its byte count comes back under this name.Both trimmed here lose real content, not just restatement:
counted's comment dropped "Giving the count back when encoding shortens the frame to match" (behaviour on the encode side), andwhen_is's comment lost its trap warning entirely — "What to send, never what to expect: a reader goes by the length it got." That's exactly the kind of non-obvious-trap comment the pruning rule says to keep.@ -21,3 +21,3 @@from .enums import known# Never a source (spec section 3.3).# Neither may be a source: spec section 3.3.This isn't a deletion, it's a rewrite: two separate comments (
# Never a source (spec section 3.3).onBROADCAST, and# Node 0 is unassigned, so it is neither a valid source nor destination.onRESERVED_NODE) were merged into one. Beyond the stated delete-only scope, it drops the actual rationale forRESERVED_NODE(why 0x00 is reserved) and the fact it's also invalid as a destination, not just a source.@ -41,8 +39,6 @@ class Priority(IntEnum):class MessageClass(IntEnum):MessageClass's docstring was its only citation of spec section 3.2, andPriorityright above kept its own "Spec section 3.1" docstring. Now inconsistent, and the 3.2 signpost is gone.@ -76,8 +75,6 @@ def interpret(@dataclass(frozen=True, slots=True)class Quantity:Quantity's docstring cited spec section 6's table and explainedfactorwith a concrete example (0.1° units → factor of ten). Nothing else in the file explains whatfactormeans; this was the only place.@ -50,10 +50,6 @@ def packed(release: str) -> int | None:def orders(version: int) -> bool:orders()'s docstring explained why a development build can be trusted to sort — it's older than every release by construction. That's non-obvious fromversion == DEVELOPMENT or release_of(version) is not Nonealone, and it was deleted outright rather than reworded.@ -64,11 +60,6 @@ def version_label(version: int) -> str:def running_label(firmware: int | None, build: int | None) -> str:running_label()'s docstring explained thatbuildis a git hash and thatNoneand0both mean "none known" — a genuine trap, since0is a falsy-but-plausible build number, not obviously equivalent to "unknown". Deleted rather than reworded.@ -220,7 +200,6 @@ class Message:@propertydef priority(self) -> int:priority's docstring cited spec sections 3.2 (class default) and 8 (per-message override) — the only place in this property that says where the fallback rule comes from.@ -234,4 +213,3 @@def varies_in_length(self) -> bool:"""Report whether what a frame carries depends on what it says."""return any(f.when for f in self.fields)carried()'s docstring noted selections are matched by number, not name, so a bare-number choice still picks out the same fields — a real behavioural detail, not a restatement of the signature.@ -108,7 +101,7 @@ class Handler(BaseHTTPRequestHandler):return self.serverdef log_message(self, format: str, *args: Any) -> None:log_message's docstring ("Keep the console for things worth reading") explained why this override is an intentional no-op — without it, a barepasshere reads as a forgotten stub rather than a deliberate silencing of default HTTP logging.@ -127,7 +120,6 @@ class Handler(BaseHTTPRequestHandler):self._json({"problem": message}, status)def _drain(self) -> None:The comment "So a kept-alive connection stays in step" explained why
_drain()exists at all — draining the unread body to keep an HTTP/1.1 keep-alive connection in sync. That's not restated by the code below it and was deleted.