Don't store plus-device in nodetypes cache #344
Conversation
|
Warning Rate limit exceeded@bouwew has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe cache save logic now filters node types to include only entries with node_type.value > 1 before persisting. Tests were updated to treat node Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Cache as Cache.save_cache
participant Store as write_cache
Caller->>Cache: save_cache()
Note over Cache: Build mapping from self._nodetypes
Cache->>Cache: Filter mapping to entries where node_type.value > 1
Note right of Cache: omit entries with value ≤ 1
Cache->>Store: write_cache(filtered_map, rewrite=True)
Store-->>Caller: Completed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
- Coverage 81.76% 81.67% -0.09%
==========================================
Files 36 36
Lines 8142 8142
==========================================
- Hits 6657 6650 -7
- Misses 1485 1492 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugwise_usb/network/cache.py (2)
30-31: Filter explicitly for CIRCLE_PLUS, not by ordinal.
node_type.value > 1also drops STICK and relies on enum ordinals. Be explicit to match the PR intent.Apply:
- cache_data_to_save: dict[str, str] = { - mac: node_type.name for mac, node_type in self._nodetypes.items() if node_type.value > 1 - } + cache_data_to_save: dict[str, str] = { + mac: node_type.name + for mac, node_type in self._nodetypes.items() + if node_type is not NodeType.CIRCLE_PLUS + }
58-63: Optional: ignore CIRCLE_PLUS on restore (backward‑compat).Prevents lingering Plus entries in memory from legacy cache files and aligns behavior across read/write.
Apply:
if node_type is None: _LOGGER.warning( "Invalid NodeType in cache for mac %s: %s", mac, node_value ) continue + if node_type is NodeType.CIRCLE_PLUS: + _LOGGER.debug("Skipping CIRCLE_PLUS entry for %s (not cached by design)", mac) + continue self._nodetypes[mac] = node_typetests/test_usb.py (1)
1718-1726: Optional: add regression for legacy CIRCLE_PLUS lines.If you adopt “skip on restore,” add a line with
NodeType.CIRCLE_PLUStomock_read_dataand assert it’s ignored.Example tweak:
- mock_read_data = [ - "0123456789ABCDEF;NodeType.CIRCLE", - "FEDCBA9876543210;CIRCLE", - "1298347650AFBECD;6", - ] + mock_read_data = [ + "0123456789ABCDEF;NodeType.CIRCLE_PLUS", # legacy cache entry + "FEDCBA9876543210;CIRCLE", + "1298347650AFBECD;6", + ] ... - assert pw_nw_cache.nodetypes == { - "0123456789ABCDEF": pw_api.NodeType.CIRCLE, - "FEDCBA9876543210": pw_api.NodeType.CIRCLE, - "1298347650AFBECD": pw_api.NodeType.SCAN, - } + assert pw_nw_cache.nodetypes == { + "FEDCBA9876543210": pw_api.NodeType.CIRCLE, + "1298347650AFBECD": pw_api.NodeType.SCAN, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/network/cache.py(1 hunks)tests/test_usb.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
📚 Learning: 2025-08-30T10:46:42.649Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/network/cache.py
🧬 Code graph analysis (2)
tests/test_usb.py (3)
plugwise_usb/network/cache.py (2)
restore_cache(42-68)nodetypes(23-25)plugwise_usb/nodes/helpers/cache.py (1)
restore_cache(53-59)plugwise_usb/api.py (1)
NodeType(62-73)
plugwise_usb/network/cache.py (1)
plugwise_usb/messages/responses.py (1)
node_type(634-636)
🔇 Additional comments (3)
tests/test_usb.py (3)
1707-1716: LGTM: test data now treats Plus as Circle in cache inputs.Matches the “don’t store Plus in nodetypes cache” policy and the new save filter behavior.
1720-1732: LGTM: restore accepts enum names and numeric values consistently.Expectations for CIRCLE and SCAN map correctly after restore.
Also applies to: 1729-1732
1739-1746: LGTM: expected write excludes Plus and preserves order.Write assertions reflect filtered cache (CIRCLE only) and stable insertion ordering.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/network/cache.py (1)
60-66: Also skip excluded types when restoring legacy cache files (prevents stale Plus entries in memory).If an older cache on disk contains
CIRCLE_PLUS, it will currently be restored into_nodetypesuntil the next save. Drop it during restore to align runtime state with the new persistence rule.Apply this diff:
@@ - if node_type is None: + if node_type is None: _LOGGER.warning( "Invalid NodeType in cache for mac %s: %s", mac, node_value ) continue + if node_type in _EXCLUDED_CACHE_TYPES: + _LOGGER.debug( + "Skipping excluded NodeType %s for mac %s during restore", + str(node_type), + mac, + ) + continue
🧹 Nitpick comments (2)
plugwise_usb/network/cache.py (2)
30-32: Replace magic threshold with explicit enum exclusions (future‑proof, self‑documenting).Relying on
node_type.value > 1is brittle if enum values change/reorder. Filter by named members instead.Apply this diff:
@@ -_LOGGER = logging.getLogger(__name__) -_NETWORK_CACHE_FILE_NAME = "nodetype.cache" +_LOGGER = logging.getLogger(__name__) +_NETWORK_CACHE_FILE_NAME = "nodetype.cache" + +# NodeTypes we never persist to disk (e.g., the Plus device and, optionally, UNKNOWN) +# Extend this set as needed rather than using numeric thresholds. +_EXCLUDED_CACHE_TYPES: set[NodeType] = { + NodeType.CIRCLE_PLUS, + # Add below if desired: + # NodeType.UNKNOWN, +} @@ - cache_data_to_save: dict[str, str] = { - mac: node_type.name - for mac, node_type in self._nodetypes.items() - if node_type.value > 1 - } + cache_data_to_save: dict[str, str] = { + mac: node_type.name + for mac, node_type in self._nodetypes.items() + if node_type not in _EXCLUDED_CACHE_TYPES + }
72-85: Don't retain excluded NodeType values in the in‑memory cache (exclude CIRCLE_PLUS)NodeType.CIRCLE_PLUS is defined in plugwise_usb/api.py and referenced by tests and plugwise_usb/nodes/init.py; in plugwise_usb/network/cache.py.update_nodetypes add an early return when node_type is an excluded value — use _EXCLUDED_CACHE_TYPES if present or define it as {NodeType.CIRCLE_PLUS} and log a debug message before returning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/network/cache.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
📚 Learning: 2025-08-30T10:46:42.649Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/network/cache.py
🧬 Code graph analysis (1)
plugwise_usb/network/cache.py (2)
plugwise_usb/nodes/node.py (1)
mac(235-237)plugwise_usb/messages/responses.py (1)
node_type(634-636)
This reverts commit 0ce3abe.
|
dirixmjm
left a comment
dirixmjm
left a comment
There was a problem hiding this comment.
As discussed, makes sense!
Summary by CodeRabbit
Bug Fixes
Tests
Chores