Change DHW comfort switch to select#883
Conversation
📝 WalkthroughWalkthroughThis PR extends the Plugwise integration to support DHW (Domestic Hot Water) comfort mode selection for heater_central entities. A new constant maps the DHW comfort measurement to a select_dhw_mode field; collection logic gathers allowed DHW modes; the appliance data pipeline is reordered to capture cooling_enabled state and derive select_dhw_mode from DHW measurements; and test fixtures are updated to reflect the new structure. ChangesDHW Mode Selection for Heater Central
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugwise/helper.py (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import.
The
TOGGLESconstant is imported but never used in this file.🧹 Proposed fix
- TOGGLES,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugwise/helper.py` at line 35, Remove the unused import TOGGLES from the import list in this module: locate the import statement that includes TOGGLES in plugwise/helper.py and delete TOGGLES (or the whole import if it becomes empty) so the file no longer imports an unused symbol.Source: Pipeline failures
tests/test_adam.py (1)
120-123:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTest will fail:
dhw_cm_switchremoved from fixture.According to the layer description, the
adam_plus_anna_newfixture (loaded at line 35) has haddhw_cm_switchentries removed from switches objects. This test attempts to tinker withdhw_cm_switchon the heater_central device and expects success (line 123assert switch_change), but the switch no longer exists in the fixture data.🔧 Suggested fix options
Option 1: Remove the obsolete switch test
- switch_change = await self.tinker_switch( - api, - "056ee145a816487eaa69243c3280f8bf", - model="dhw_cm_switch", - ) - assert switch_changeOption 2: Update test to verify the new select entity
Replace the switch test with a test for the new
select_dhw_modefunctionality, if a corresponding test helper exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_adam.py` around lines 120 - 123, The test fails because the adam_plus_anna_new fixture no longer includes the dhw_cm_switch entity; update tests/test_adam.py by removing the obsolete switch assertion or replacing it with a check for the new select entity (e.g., change the call that targets model="dhw_cm_switch" on device "056ee145a816487eaa69243c3280f8bf" to instead target the new select entity name such as "select_dhw_mode" and assert its behavior), ensuring you update the invoked test helper call and the final assertion (currently "assert switch_change") to match the new entity and expected return value.
🧹 Nitpick comments (1)
plugwise/helper.py (1)
467-480: ⚡ Quick winClarify
old_measurementinitialization and scope.The variable
old_measurementis assigned on line 468 only whennew_nameexists, but it's referenced unconditionally on line 477. While this appears safe in practice (bothdomestic_hot_water_modeanddomestic_hot_water_comfort_modehave DATA mappings with names), the logic is fragile and not immediately obvious.♻️ Proposed refactor for clarity
def _appliance_measurements( self, appliance: etree.Element, data: GwEntityData, measurements: dict[str, DATA | UOM], ) -> None: """Helper-function for _get_measurement_data() - collect appliance measurement data.""" for measurement, attrs in measurements.items(): p_locator = f'.//logs/point_log[type="{measurement}"]/period/measurement' if (appl_p_loc := appliance.find(p_locator)) is not None: if skip_obsolete_measurements(appliance, measurement): continue + old_measurement = measurement if new_name := getattr(attrs, ATTR_NAME, None): - old_measurement = measurement measurement = new_name match measurement: case "elga_status_code": data["elga_status_code"] = int(appl_p_loc.text) case "select_dhw_mode": if self._dhw_allowed_modes: data["select_dhw_mode"] = appl_p_loc.text if old_measurement == "domestic_hot_water_comfort_mode": data["select_dhw_mode"] = ( "comfort" if appl_p_loc.text == "on" else "off" )This ensures
old_measurementis always defined before the match statement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugwise/helper.py` around lines 467 - 480, old_measurement is only set when new_name exists but is referenced later unconditionally in the match branch for "select_dhw_mode"; initialize old_measurement to the current measurement before the conditional that may rename it so it always has a value (ensure the block using ATTR_NAME still updates measurement when new_name is present and preserves old_measurement for later checks), referencing the existing symbols measurement, old_measurement, ATTR_NAME, appl_p_loc, and self._dhw_allowed_modes to locate and adjust the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugwise/helper.py`:
- Around line 267-278: _collect_dhw_modes calls _get_toggle_state with None for
the data parameter which violates the declared GwEntityData (non-Optional) type;
either make _get_toggle_state accept Optional[GwEntityData] (update its
signature and handle None inside the function) or change the call in
_collect_dhw_modes to pass an empty dict/appropriate GwEntityData-shaped empty
value instead of None for the "domestic_hot_water_comfort_mode" /
"dhw_cm_switch" call so the types remain consistent.
---
Outside diff comments:
In `@plugwise/helper.py`:
- Line 35: Remove the unused import TOGGLES from the import list in this module:
locate the import statement that includes TOGGLES in plugwise/helper.py and
delete TOGGLES (or the whole import if it becomes empty) so the file no longer
imports an unused symbol.
In `@tests/test_adam.py`:
- Around line 120-123: The test fails because the adam_plus_anna_new fixture no
longer includes the dhw_cm_switch entity; update tests/test_adam.py by removing
the obsolete switch assertion or replacing it with a check for the new select
entity (e.g., change the call that targets model="dhw_cm_switch" on device
"056ee145a816487eaa69243c3280f8bf" to instead target the new select entity name
such as "select_dhw_mode" and assert its behavior), ensuring you update the
invoked test helper call and the final assertion (currently "assert
switch_change") to match the new entity and expected return value.
---
Nitpick comments:
In `@plugwise/helper.py`:
- Around line 467-480: old_measurement is only set when new_name exists but is
referenced later unconditionally in the match branch for "select_dhw_mode";
initialize old_measurement to the current measurement before the conditional
that may rename it so it always has a value (ensure the block using ATTR_NAME
still updates measurement when new_name is present and preserves old_measurement
for later checks), referencing the existing symbols measurement,
old_measurement, ATTR_NAME, appl_p_loc, and self._dhw_allowed_modes to locate
and adjust the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3649d18-c70a-480a-8b38-f2124773ded9
📒 Files selected for processing (10)
plugwise/constants.pyplugwise/helper.pytests/data/adam/adam_bad_thermostat.jsontests/data/adam/adam_heatpump_cooling.jsontests/data/adam/adam_jip.jsontests/data/adam/adam_onoff_cooling_fake_firmware.jsontests/data/adam/adam_plus_anna.jsontests/data/adam/adam_plus_anna_new.jsontests/data/adam/adam_plus_anna_new_regulation_off.jsontests/test_adam.py
1 participant
Summary by CodeRabbit
New Features
Tests