fix/perf: code audit — dead code, apply_odu, close() token, wire map constants, shared refresh callback, id_variants#10
Conversation
… constants, shared refresh callback, id_variants dedup - Remove unreachable None-guards in SpaceControls.display_setpoint_str() (temperature_setpoint_c is float, not float|None; 6 dead lines confirmed by coverage) - Fix apply_outdoor_unit to use the updates-dict + single replace() pattern consistent with all other apply_* methods in SystemSnapshot - Clear self._token in QuiltClient.close() so get_current_token() does not return a stale token after the client is closed - Hoist FanSpeed and LouverAngle wire-encoding dicts to module-level constants (_FAN_SPEED_WIRE_MAP, _LOUVER_ANGLE_WIRE_MAP) so to_wire() no longer re-allocates them on every call - Extract _invoke_refresh_callback from transport.py and streaming.py into a single invoke_refresh_callback in tokens.py; streaming's copy lacked the WeakKeyDictionary cache (called inspect.signature on every refresh event); update all callers and tests - Move _id_variants from system.py into _helpers.py and reuse it inside lookup_hardware, eliminating duplicated ID-normalisation logic - Change invalidate_snapshot() log level from WARNING to DEBUG
There was a problem hiding this comment.
Pull request overview
This PR is a broad audit/refactor pass across the async Quilt client library, focusing on removing dead code, reducing per-call allocations and introspection overhead, de-duplicating ID-normalization logic, and tightening lifecycle correctness (token clearing on close).
Changes:
- Centralize refresh-callback invocation into
tokens.invoke_refresh_callback()with signature caching; update transport/streaming and tests to use it. - Reduce overhead in hot paths: hoist enum wire mappings to module constants and streamline
SystemSnapshot.apply_outdoor_unit()to a singlereplace(). - De-duplicate ID normalization (
_id_variants) and clean up minor behavior/logging + update CHANGELOG.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_transport.py | Update tests to call shared invoke_refresh_callback. |
| tests/test_transport_interceptor_extra.py | Point signature-caching tests at tokens.invoke_refresh_callback + shared cache. |
| tests/test_streaming.py | Import/alias the shared refresh-callback invoker for streaming tests. |
| src/quilt_hp/transport.py | Remove local _invoke_refresh_callback and delegate to tokens.invoke_refresh_callback. |
| src/quilt_hp/tokens.py | Add shared invoke_refresh_callback implementation + signature cache. |
| src/quilt_hp/services/streaming.py | Remove local _invoke_refresh_callback and delegate to shared implementation. |
| src/quilt_hp/models/system.py | Import _id_variants from helpers; refactor apply_outdoor_unit() to single replace(). |
| src/quilt_hp/models/space.py | Remove unreachable fallback logic in display_setpoint_str(). |
| src/quilt_hp/models/enums.py | Hoist fan/louver wire maps to module-level constants. |
| src/quilt_hp/models/_helpers.py | Add _id_variants helper and refactor lookup_hardware() to use it. |
| src/quilt_hp/client.py | Lower invalidate snapshot log level; clear token on close(). |
| CHANGELOG.md | Document the fixes/changes under Unreleased. |
| def lookup_hardware(hw_map: dict[str, object], hardware_id: str | None) -> object | None: | ||
| """Resolve hardware objects across common ID formats.""" | ||
| if not hardware_id: | ||
| return None | ||
| raw = hardware_id.strip() | ||
| if not raw: | ||
| return None | ||
| keys = ( | ||
| raw, | ||
| raw.rsplit("/", 1)[-1], | ||
| raw.rsplit(":", 1)[-1], | ||
| raw.casefold(), | ||
| raw.rsplit("/", 1)[-1].casefold(), | ||
| raw.rsplit(":", 1)[-1].casefold(), | ||
| ) | ||
| for key in keys: | ||
| for key in _id_variants(hardware_id): | ||
| hw = hw_map.get(key) | ||
| if hw is not None: |
…nvoke_refresh_callback docstring - Introduce _id_variant_keys() returning a deterministic tuple (exact → tail-slash → tail-colon → casefold variants); lookup_hardware iterates this sequence so match priority is always exact > tail > casefold, matching the original implementation. _id_variants() still returns a set (used for set-intersection in SystemSnapshot) but now calls _id_variant_keys() internally so normalization logic is defined once. - Clarify invoke_refresh_callback docstring: what is cached is whether the callback accepts a TokenRefreshContext argument (the signature introspection result), not the callback's return value.
eman
left a comment
There was a problem hiding this comment.
Both comments addressed in f10c502. lookup_hardware now uses _id_variant_keys() for deterministic priority order (exact, tail-slash, tail-colon, casefold); _id_variants() also calls it internally so normalization is defined once. Docstring updated to clarify it is the signature introspection result that is cached.
Summary
Full code audit pass. All 234 tests pass, ruff clean, mypy clean before and after.
Fixes
Bugs
SpaceControls.display_setpoint_str()(models/space.py) — removed 6 lines of dead code in the fallback branch.temperature_setpoint_cis typedfloat, making theif best is None:guards permanently unreachable. Confirmed by coverage (lines never hit).SystemSnapshot.apply_outdoor_unit()(models/system.py) — calleddataclasses.replace()up to twice in sequence, creating an intermediate object when both conditions held. Refactored to collect patches into anupdatesdict and callreplace()once, consistent with all otherapply_*methods.QuiltClient.close()(client.py) — did not clearself._token, leaving a stale token accessible viaget_current_token()after the channel was closed. Addedself._token = None.Performance
FanSpeed.to_wire()/LouverAngle.to_wire()(models/enums.py) — mapping dicts were defined as local variables, re-allocated on every call. Hoisted to module-level constants (_FAN_SPEED_WIRE_MAP,_LOUVER_ANGLE_WIRE_MAP)._invoke_refresh_callback(tokens.py,transport.py,services/streaming.py) — the streaming copy lacked theWeakKeyDictionarysignature cache present in the transport copy, causinginspect.signature()to be called on every token-refresh event. Extracted a single sharedinvoke_refresh_callbackintokens.pywith the cache; both callers import from there.Duplication
_id_variants— lived inmodels/system.pyand duplicated the same strip/rsplit/casefold logic already inmodels/_helpers.py:lookup_hardware. Moved to_helpers.pyandlookup_hardwarenow calls it.Minor
QuiltClient.invalidate_snapshot()log levelWARNING→DEBUG. Routine cache invalidation is not a warning condition.Test / lint