Remove switch-groups #829
Remove switch-groups #829
Conversation
Use the HA Group helper to create a group of switches
|
Warning Rate limit exceeded@bouwew has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 26 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)
WalkthroughThis PR removes switching-group support: deletes group fixtures and test entries, drops GROUP_TYPES and switching-group helpers, and removes members-based group handling (including the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Tests / Caller
participant API as plugwise.__init__.py (API)
participant Smile as plugwise/smile.py (SmileAPI)
Test->>API: await set_switch_state(appl_id, model, state)
API->>Smile: set_switch_state(appl_id, model, state)
Note right of Smile: group-member branches removed\n(now handles single switch only)
Smile-->>API: bool (success/failure)
API-->>Test: return bool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
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 #829 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3451 3403 -48
=========================================
- Hits 3451 3403 -48 ☔ 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 (2)
plugwise/data.py (1)
192-203: OpenTherm availability gate is reasonable; name check is a bit magicRestricting the OpenTherm availability check to
entity["name"] != "OnOff"avoids incorrectly marking pure on/off heaters unavailable on “no OpenTherm communication” messages, which matches the PR intent. The only minor concern is the hard dependency on the literal name"OnOff"; if that label ever changes, this gate will silently break. Consider tying this to a capability flag instead if you touch this area again.plugwise/smile.py (1)
396-438: Per-switch set_switch_state implementation matches new API contractThe per-entity switch handling (including model-specific handling via
model_to_switch_items, actuator lookup, and relay lock guard) is consistent with the new no-group API and with the test helpers’ expectations. The remaining assumption is that at least one matching actuator exists soswitch_idis always set, which is unchanged behavior but worth remembering if new device types are added.If you later expand device support, consider adding a defensive fallback (e.g. raising a
PlugwiseError) when no matching actuator is found instead of relying on that invariant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
fixtures/adam_multiple_devices_per_zone/data.json(0 hunks)fixtures/adam_plus_anna_new/data.json(0 hunks)fixtures/adam_plus_anna_new_regulation_off/data.json(0 hunks)fixtures/m_adam_cooling/data.json(0 hunks)fixtures/m_adam_heating/data.json(0 hunks)fixtures/m_adam_multiple_devices_per_zone/data.json(0 hunks)fixtures/stretch_v23/data.json(0 hunks)fixtures/stretch_v31/data.json(0 hunks)plugwise/__init__.py(2 hunks)plugwise/common.py(2 hunks)plugwise/constants.py(0 hunks)plugwise/data.py(1 hunks)plugwise/legacy/data.py(0 hunks)plugwise/legacy/smile.py(2 hunks)plugwise/smile.py(2 hunks)tests/data/adam/adam_multiple_devices_per_zone.json(0 hunks)tests/data/adam/adam_plus_anna_new.json(0 hunks)tests/data/adam/adam_plus_anna_new_UPDATED_DATA.json(0 hunks)tests/data/adam/adam_plus_anna_new_regulation_off.json(0 hunks)tests/data/stretch/stretch_v23.json(0 hunks)tests/data/stretch/stretch_v31.json(0 hunks)tests/data/stretch/stretch_v31_UPDATED_DATA.json(0 hunks)tests/test_adam.py(2 hunks)tests/test_init.py(3 hunks)tests/test_legacy_stretch.py(2 hunks)
💤 Files with no reviewable changes (17)
- tests/data/stretch/stretch_v23.json
- fixtures/adam_multiple_devices_per_zone/data.json
- tests/data/stretch/stretch_v31_UPDATED_DATA.json
- tests/data/adam/adam_plus_anna_new_regulation_off.json
- fixtures/m_adam_heating/data.json
- plugwise/constants.py
- tests/data/adam/adam_multiple_devices_per_zone.json
- plugwise/legacy/data.py
- fixtures/adam_plus_anna_new/data.json
- fixtures/m_adam_cooling/data.json
- fixtures/stretch_v23/data.json
- fixtures/adam_plus_anna_new_regulation_off/data.json
- tests/data/stretch/stretch_v31.json
- fixtures/m_adam_multiple_devices_per_zone/data.json
- tests/data/adam/adam_plus_anna_new_UPDATED_DATA.json
- tests/data/adam/adam_plus_anna_new.json
- fixtures/stretch_v31/data.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-13T11:26:00.100Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 671
File: plugwise/legacy/data.py:97-106
Timestamp: 2024-12-13T11:26:00.100Z
Learning: In `plugwise/legacy/data.py`, the legacy thermostat does not support cooling, so handling of the cooling state is unnecessary in this code.
Applied to files:
plugwise/data.py
📚 Learning: 2025-11-09T09:22:20.172Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 814
File: plugwise/constants.py:254-262
Timestamp: 2025-11-09T09:22:20.172Z
Learning: In `plugwise/constants.py`, `ACTIVE_KEYS` contains only the keys that exist as original actuators in the raw Plugwise device data. Keys like `setpoint_high` and `setpoint_low` are NOT original actuators—they are derived/translated later for Home Assistant integration, so they should not be included in `ACTIVE_KEYS` even though they appear in the `ActuatorData` TypedDict.
Applied to files:
plugwise/common.py
🧬 Code graph analysis (3)
plugwise/legacy/smile.py (2)
plugwise/__init__.py (1)
set_switch_state(411-427)plugwise/smile.py (1)
set_switch_state(396-438)
tests/test_init.py (2)
plugwise/__init__.py (1)
set_switch_state(411-427)plugwise/smile.py (1)
set_switch_state(396-438)
plugwise/__init__.py (2)
plugwise/legacy/smile.py (1)
set_switch_state(218-265)plugwise/smile.py (1)
set_switch_state(396-438)
🔇 Additional comments (8)
plugwise/common.py (1)
176-210: Pumping-only group handling looks consistent with removal of switch-groupsFiltering on
group_type != "pumping"and requiring non-emptymembersbefore adding a group keepsgw_entitiesaligned with real pumping groups and avoids orphaned/empty groups. The docstring matches the new behavior; no issues spotted here.tests/test_legacy_stretch.py (1)
32-32: Adjusted entity counts align with group-removalLowering
self.entity_itemsexpectations in the Stretch v3.1 and v2.3 tests is consistent with removing switch-group entities from the data model. As long as the fixtures were regenerated after the group removal (which they appear to be), these new counts look correct.If you haven’t already, please run the Stretch tests once more after any further fixture tweaks to ensure these counts stay in sync.
Also applies to: 70-72
plugwise/__init__.py (1)
411-427: Public API signature change is coherent with backend changesThe simplified
set_switch_state(appl_id, model, state)wrapper and its validation look correct and line up with the new Smile/SmileLegacy implementations. The only thing to keep in mind is that this is a breaking signature change for any external callers still passingmembers; ensure dependent projects (e.g., HA integration) are updated in lockstep or gated via versioning.plugwise/smile.py (1)
102-121: Group docstring now correctly reflects pumping-only behaviorUpdating the comment to “Collect and add pumping-group entities” matches the narrowed
_get_groups()behavior inSmileCommon; nothing else to flag here.tests/test_adam.py (1)
35-38: Adam entity count expectations updated appropriatelyThe reduced
self.entity_itemsexpectations in the Adam+Anna and multi-device tests line up with the removal of switch-groups, while still validating the overall topology. As long as the fixtures behind these setups were regenerated after the group removal, these new numbers look sound.It’s worth re-running the Adam test suite once more after any subsequent fixture tweaks to ensure these counts remain correct.
Also applies to: 222-225, 297-299
plugwise/legacy/smile.py (2)
77-85: Legacy entity collection docstring matches simplified behaviorThe updated description of
get_all_gateway_entitiesfor legacy devices (no mention of groups) matches the fact that group data is no longer merged intogw_entities. No issues here.
218-265: Legacy set_switch_state aligned with new signature and behaviorThe legacy
set_switch_state(appl_id, model, state)keeps the previous per-relay and lock logic, while dropping group handling to match the new public API. For Stretch v2 handling (_stretch_v2) you still correctly switch actuator namespaces. This looks consistent with the tests intests/test_legacy_stretch.py.tests/test_init.py (1)
624-650: Test helpers now match the simplified set_switch_state APIUpdating
tinker_switchandtinker_switch_bad_inputto callapi.set_switch_state(dev_id, model, new_state)without amembersargument correctly reflects the new core API and still validates both happy-path behavior and bad-input error handling. The signatures and call sites in other tests remain consistent with this change.Also applies to: 653-665
bouwew
force-pushed
the
remove-sw-group
branch
from
d2c86b2 to
9e9474c
Compare
|
|
Not merged to keep the present functionality in place for non-HA platforms. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.