Fix three code-smell comments in updater and firmware port #105
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/updater-status-and-port"
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?
Three related cleanups, each removing a comment that existed because the code was awkward:
UpdateStatus.versionis nowstr | None, dropping the empty-string sentinel and theor ""/or Noneconversions inupdater/link.pyandupdater/api.py.FirmwareCache.images()sorts once instead of twice, with a combined(environment, version-descending)key.CONTROL_API_PORTis now defined once incustom_components/campervan/protocol/__init__.pyand imported by bothupdater/config.pyand the integration'sfirmware.py, replacing the duplicatedUPDATER_PORT = 8787and the test that kept the two literals in step.Behaviour unchanged;
tools/checkis green (1222 passed).Review (can't leave a formal review on my own PR, so posting as a comment):
updater/firmware.py, theimages()sort (blocking) — the one-pass sort isn't behaviourally equivalent to the old two-pass sort.It differs when two images in the same environment have version strings with a different number of dot-separated segments (one a proper prefix of the other) —
version_orderdoesn't require fixed arity, so nothing rules this out.Elementwise negation correctly reverses comparison between equal-length tuples, but Python's tuple comparison also has a "shorter is less" rule when one tuple is a prefix of another, and negation doesn't flip that rule (a prefix stays a prefix, whatever the sign of its elements). Concrete example, same environment:
"2.1"→version_order=(2, 1)"2.1.5"→version_order=(2, 1, 5)Old code (
reverse=Trueoverversion_order):(2,1) < (2,1,5), so reversed puts"2.1.5"first — newest first, correct.New code (negate, then ascending):
(-2,-1) < (-2,-1,-5)— still a prefix relation, unaffected by negation — so ascending puts"2.1"first. The shorter version now sorts as "newest", which is backwards.In practice every version that reaches the cache today is a 3-part CalVer string, so this is probably dormant — but the
ASSETregex's version group ([^-]+) doesn't itself enforce that arity, so nothing guarantees it. Either add a comment naming the fixed-arity assumption this key now relies on, or fix the key so it doesn't depend on negation reversing prefix order, and add a test with two same-environment versions of different segment counts.Minor, non-blocking:
CONTROL_API_PORTnow lives incustom_components/campervan/protocol/__init__.py, whose module docstring says "The wire format, in Python." A control-API port is a deployment detail, not part of the wire format — a small conceptual mismatch with where it's homed, even though the one-way-dependency reasoning for putting it there is sound.Everything else checks out: the
UpdateStatus.versionsentinel removal is complete and consistent (no leftover""-assuming code paths), and thefirmware.pydiff only touches theUPDATER_HOST/UPDATER_PORTblock plus its import line, respecting the concurrent-edit constraint.