Trim verbosity across the codebase #79
Loading…
Reference in a new issue
No description provided.
Delete branch "refactor/trim-verbosity"
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?
Same behaviour, materially less code. Comments and docstrings are cut back to what the code cannot say for itself — spec citations, units, sentinel rules and real traps stay, one line each — and a handful of copied-out patterns fold into one helper apiece, with a few hot-path efficiency fixes that fell out on the way (cached shapes and identifier routing, a keyed alarm store, one manifest pass per request, raw-id pre-filters).
Tests are deliberately untouched: the existing 1123 are the gate for a refactor that must not change behaviour. Across
custom_components,updaterandtoolsthe three trees go from 18,411 lines to 16,492.A few places where the result is not a pure deletion:
entity_shape.py: the semantic-class guards readisinstance(known, SemanticClass) and known in Xrather than a bare membership test, which is what mypy accepts.event.py: theEVENT_TYPESconstant stays for RUF012 and mypy; only the property that returned it went.serve_http(updater) callsserver_close()after awaiting the serve task, which was the control API's original order. The file server used to close first.session.run(updater) logs a hotspot failure at WARNING without a traceback, where it used to useexception.readings.notifylogs a listener that raises under one logger, where the six copies it replaces each used their own module's./api/catalogueno longer emitsoffset,transmitted,messageTypeorrules. Nothing instatic/app.jsreads any of them; every other key and value is byte-identical, checked by diffing the rendered JSON against this branch's base.ipaddress.IPv4Address, so192.168.001.10is now refused as ambiguous, and a bad address gets one message rather than three specific ones.pyproject.tomlstops requiring a docstring on a function, method or__init__; a module and a class still need one.220c83040cb71645ed69Gate green (1123 passed, ruff and mypy clean). Bench JSON verified identical to base except the four intentionally dropped keys (
offset,transmitted,messageType,rules), none of whichstatic/app.jsreads. Findings:protocol/units.pysentinel_at— a bareKeyErrorwhere there wasNoneor a named refusal._BY_PATTERN[signed, width]raisesKeyErrorfor any shape not in the table. Old behaviour:(False, 1)and(False, 8)returnedNone;(True, 4)raisedValueError("the specification reserves no signed 32-bit values"). That message was a spec statement and it is gone. Not reachable today — every fieldmeasurement.measurement_ofroutes tointerpretis width 2 or unsigned 4 — but a width-1 quantity field added to the catalogue later would raise inside frame decoding instead of returning nothing. Keep the explicit refusal for signed-32 and fall back to{}for the rest.Related, same function:
raw % (1 << width * 8)now names a sentinel for a raw value wider than the field (sentinel_at(0x1FFFF, signed=False, width=2)isUNKNOWN, wasNone). Also unreachable, also silent if it ever is not.entity.py:154—self._keyis cached in__init__butasync_redescribed(line 167) reassignsself.endpointwithout it. Correct today only becauseplatforms.settlelooks the entity up by key before calling it, so the new endpoint always has the same key. Nothing states or enforces that. Set_keyalongsideself.endpoint, or drop the cache.**
updater/firmware.py:344_destination** — the new docstring, "scheme and host, lower-cased, as urllib does not", says both get lower-cased here. Only the host does; urllib already lower-cases the scheme, which is what the old comment said. It also dropped the reason foror ""(a URL with no//` has no host).traffic.py:1— the module docstring lost the reason traffic reads the identifier and never decodes a payload: a second subscriber pulling frames apart is a second chance to disagree with the codec. That is the drift risk the repository is built around and the code cannot carry it. Worth one line back.**
updater/firmware.py:79FETCH_STOP** — lost "Nothing clears it: the next process starts with an event of its own." A module-globalthreading.Event` that is never reset is exactly the trap a future editor (or a test) falls into.**
updater/file_server.py:31image_path** — lost "only the newest image per node type is offered, so the version and the environment name it unambiguously". That uniqueness is what makesoffers()` safe to key on the path.**
tools/frame_bench/codec.py:154_address** — three specific messages collapsed to one, and the parser got stricter.192.168.1.300said "'300' is not between 0 and 255" and now says "Write this as an address, like 192.168.1.10.";192.168.001.10used to encode and now refuses (IPv4Address` rejects leading zeros). Fine if deliberate, but it is an operator-facing change, not just a trim.**
coordinator.py:188_availability_changed** — routing throughnotifymeans an exception in an availability listener is now logged rather than propagated into the transport's_announce`. Better behaviour, but it is a behaviour change in one of the thin-cover areas and no test pins it either way.tools/frame_bench/server.py:161—mimetypes.guess_file_typereads the host's/etc/mime.typeswhere the old three-entry table could not vary. Right answers on this machine; a host with an odd.jsmapping now breaks the page.Nits:
readings.py:52subscribe's docstring restates the signature exactly — withD1off it can go.updater/session.py:41-43— the"""The longest name and key..."""docstring sits underMAX_KEYso it reads as documenting only the key, and duplicates the#comment aboveMAX_SSID.updater/session.py:215— a hotspot failure now logs WARNING without a traceback where it logged ERROR with one;radio.pycalls a Pi left as an access point the failure that matters, andstr(failure)` is all that is left of it.Taken the review. Four fixes pushed:
sentinel_atanswers for every field shape again, and no longer wraps a raw value wider than its field. The signed-32-bitValueErroris back, spec statement and all (6ed11df).async_redescribednow refreshes_keywith the endpoint it just took (74d46b9)._destination's docstring says what the code does: urllib lower-cases the scheme, not the host (99c7e47).traffic.pydecodes no payload, that nothing clearsFETCH_STOP, and that only the newest image per node is offered soimage_pathis unique (99c7e47).Two nits also taken:
readings.subscribe's docstring and theMAX_KEYdocstring that was attached to the wrong name (692b20c).Two I am not changing:
The address parser.
ipaddress.IPv4Addressrefusing192.168.001.10is deliberate upstream behaviour, not an accident — leading zeros are ambiguous between decimal and octal, and every parser that guessed has been a source of SSRF bugs. The bench is where a frame is typed by hand before it goes on a real van's bus, so refusing the ambiguous form and asking for192.168.1.10is the answer I want. The cost is real and I have noted it: three specific messages became one, so it no longer says which octet is wrong.Static content types.
mimetypescarries.html,.cssand.jsin its own built-in table;/etc/mime.typesonly adds to it. A host without that file still serves the three files this directory holds correctly — verified against a running bench, all three headers byte-identical to what the old dict produced, charset included.One thing outside this branch:
feat/firmware-updateshas moved on since the branch was cut (#78), and the PR now conflicts in six files. I have deliberately left that alone — resolving it means merging a feature branch's behaviour into a refactor branch, which is your call rather than mine.Second round. The four fixes are good:
sentinel_atis byte-identical to the base for every(signed, width, raw)I could throw at it including negatives, oversized raws and widths 1/3/8;_keynow follows a redescribe andbinary_sensor's alarm test at line 98 rides on it. The bench's rendered catalogue JSON is identical to the base for every message except the four keys deliberately dropped (messageType,rules,offset,transmitted), andapp.jsreads none of them. Built frames, saved examples, and every message's priority and field layout come out identical.New findings, worst first.
tools/frame_bench/protocol.py:1320— a spec citation that did not exist before.# Section 13 lifts a hold above the other commands.Section 13 is cited nowhere onfeat/firmware-updatesand nowhere else in this repo; the base just hadpriority=1with no comment. If that came fromdocs/can-protocol.md, fine. If it was reasoned back from the number, it is inferring the protocol from the code and will read as authoritative to the next person. Confirm or drop it.pyproject.toml:17—ignore = ["D1", ...]is wider than the change needed.D1is the whole missing-docstring family, so D100 (module), D101 (class) and D104 (package) are off as well. A new module or class can now land with no docstring at all.ignore = ["D102", "D103", "D105", "D107", "D206", "E101", "W191"]passes on this tree as it stands — I ran it — and keeps the three that matter.custom_components/campervan/firmware.py:219— an empty refusal now raises an empty error. The base was_refusal(payload) or unexplained, so{"error": ""}fell through tothe updater answered POST /x with 500.refusal if isinstance(refusal, str) else unexplainedreturns the empty string instead. Userefusal or unexplainedafter the isinstance narrowing.tools/frame_bench/protocol.py:69-70—offsetandwidthgained defaults onFrameField. Both were required on the base and the fields are now positional, so a transcription that omits an offset silently encodes at byte 0 rather than raisingTypeError. In the one file whose job is to not drift from the specification that is the wrong default. Drop= 0from the baseoffsetand putoffset: int = 0back onQuantityField, which is the only declaration site that omits it.Nits:
custom_components/campervan/readings.py:52—notifycollapses four messages ("An alarm listener would not run", "A node listener…", "A reading listener…", "A traffic listener…") into one, logged undercampervan.readingsrather than the store's own module. Take the subject as an argument, or log from the caller.updater/session.py:215— foldingRadioErrorinto theBusErrorbranch demotes "no hotspot" from_LOGGER.exceptionto awarningwith no traceback. TheSessionResultis unchanged, so this is diagnostics only, but a hotspot that will not come up is the failure worth a traceback on a van with no screen.updater/file_server.py:73— theoffersparameter ofmanifest_bodyshadows the module-leveloffers()nine lines above. Rename itadvertised.custom_components/campervan/lockout.py:41—ends + timedelta(microseconds=-ends.microsecond % 1_000_000)is correct (checked against the old form over 20k random times) but the modular-negation trick needs a second read where the three lines it replaced did not.Round two taken. Six commits:
tests/test_frame_bench.py:71. The specification putsCMD_HOLDat priority 1 in its §8.3 catalogue entry, not in §13, so the comment now says §8.3 (b042b9a). Checked againstdocs/can-protocol.mdin the firmware repository, line 564.ignoreis now["D102", "D103", "D105", "D107", …]rather than the whole ofD1, so a new module or class still has to carry a docstring (ce33dbc).{"error": ""}falls back to the status line again, as_refusal(…) or unexplaineddid (bbe9702).FrameField.offsetis required again. Only the oneQuantityFieldlegitimately sat at zero, and it now says so (adc2c46). The rendered catalogue JSON is unchanged.advertised, so it no longer shadowsoffers(), and the lockout round-up gets a name (2c9729d).Two nits declined:
readings.notifylogging under one logger. That is what folding six copies into one helper costs. Every one of the six said the same thing and no test reads the text; the stack in the traceback still names the listener. Splitting it back apart to keep six module loggers would undo the change.session.run's hotspot failure at WARNING without a traceback. Deliberate and already listed in the description. A hotspot that will not come up is an operational fact, not a bug in the updater, and the failure's own message says which step failed.Round three. The six fixes are correct; §8.3 checks out against
docs/can-protocol.md:564, and the bench's rendered catalogue JSON is a strict subset of the base branch's (onlyoffset,transmitted,messageTypeandrulesdropped, not one value changed). Nothing blocking. Three wording points, the first of which is a comment that now says the wrong thing.@ -99,4 +87,1 @@Only `cleared` clears it. Reading a state this build has no word foras off is the one mistake a gas alarm may not make."""return self.state != AlarmState.CLEAREDNit. The reason for
!=rather than a membership test went with the docstring — reading a state this build has no word for as off is the one mistake a gas alarm may not make. The module docstring still carries the rule, but the invariant is now four screens from the line that depends on it, andself.state in (RAISED, ACTIVE)looks like a tidy-up. One trailing comment would hold it.@ -207,3 +161,3 @@@callbackdef _frame_arrived(self, message: can.Message) -> None:"""Hand one frame to everyone listening."""Hand one frame to everyone listening, counting it once if none could."counting it once if none could" inverts the rule.
unusableis set by any listener raising, so the frame is counted once however many listeners could not use it — not only when none could. The old wording had it right; the summary just needs to say "however many listeners could not".@ -10,3 +7,1 @@none and has no descriptors to serve; a node asking `QRY_DESCRIPTORS` getsnothing back from here, which spec section 8.7 says it should and nothing heredoes yet.It owns no endpoints, so it serves no descriptors, which spec section 8.7 saysThe "which" now attaches to "serves no descriptors", so it reads as though §8.7 endorses serving none. The point was the opposite: a node asking
QRY_DESCRIPTORSgets nothing back from here, which §8.7 says it should get. NamingQRY_DESCRIPTORSputs it back in one line.All three taken in
bac9afe:_frame_arrivedsays "counting it once if any could not", which is what the code does.presence.pysays what §8.7 is actually about: a node askingQRY_DESCRIPTORSgets nothing back from here.Alarm.is_raisedhas its reason back, on the line that depends on it: onlyclearedclears it, and reading an unnamed state as off is the one mistake a gas alarm may not make.Gate green. Three rounds, nothing left outstanding on my side.
Reviewed the merge (
dfd0bbc) and the trim that followed (b21af58). Nothing to act on.Verified: with docstrings and comments stripped, the code delta the merge adds is identical to #78's own, across all eight touched source files — the only difference is the deliberate
except (RadioError, BusError)flatten inupdater/session.py. Tests, vectors and README are byte-identical to #78's tip, and the branch never touched tests. No trimmed docstring still describes the pre-#78 model; nouint16-cannot-be-ordered prose survives anywhere. Gate re-run clean: ruff, format, mypy strict, 1164 passed.