Store the last_used_schedule for reuse #945
Conversation
WalkthroughAdded a dataclass to persist extra climate restore data and made the Plugwise climate entity a RestoreEntity; implemented restore/load of Changes
Sequence Diagram(s)sequenceDiagram
participant HA as Home Assistant
participant Entity as PlugwiseClimateEntity
participant API as Plugwise Device/API
Note over Entity: Restore on startup
HA->>Entity: async_added_to_hass()
Entity->>Entity: load extra_restore_state_data -> _last_active_schedule, _previous_action_mode
rect #f0f7ff
Note over HA,Entity: Mode change with schedule propagation
HA->>Entity: async_set_hvac_mode(mode [, selected_schedule])
Entity->>Entity: determine desired_schedule (selected or _last_active_schedule)
alt schedule present
Entity->>API: set_schedule_state(mode, ..., desired_schedule)
API-->>Entity: result
Entity->>Entity: update _previous_action_mode & _last_active_schedule
else schedule missing but required
Entity-->>HA: raise HomeAssistantError (ERROR_NO_SCHEDULE)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-23T16:26:34.700ZApplied to files:
🧬 Code graph analysis (2)tests/components/plugwise/test_climate.py (2)
custom_components/plugwise/climate.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/climate.py(6 hunks)tests/components/plugwise/test_climate.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 GitHub Check: CodeFactor
custom_components/plugwise/climate.py
[notice] 111-111: custom_components/plugwise/climate.py#L111
Multiple spaces before operator. (E221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Prepare
🔇 Additional comments (2)
tests/components/plugwise/test_climate.py (2)
107-109: Schedule name is correct and consistent with test fixtures.The verification confirms that
"GF7 Woonkamer"(with two spaces) is the correct schedule name from the fixture data. The device IDc50f167537524366a5af7aa3942feb1ein the test matches the fixture entry where"select_schedule": "GF7 Woonkamer"is defined. The two-space formatting is intentional and consistent across the fixture's available schedules and device configuration.
336-338: No changes needed—test fixture data confirms "standaard" is the correct schedule name.The verification confirms that "standaard" is the correct active schedule name for the Anna climate entity. The fixture data across multiple Anna configurations (m_anna_heatpump_cooling, m_anna_heatpump_idle, anna_heatpump_heating) consistently shows
"select_schedule": "standaard"and lists it in available schedules, matching the test assertion.
bouwew
force-pushed
the
save_last_option
branch
from
603ade8 to
0089a5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
custom_components/plugwise/climate.py (2)
27-27: RestoreEntity inheritance remains unused.This duplicates a previous review comment.
RestoreEntityis imported and inherited butasync_added_to_hassis never overridden to restore state. Since_last_active_scheduleis populated from coordinator data in__init__and during property access, and coordinator data is refreshed on Home Assistant restarts, theRestoreEntityinheritance serves no purpose and should be removed.Remove the import at line 27 and remove
RestoreEntityfrom the inheritance list at line 102.Also applies to: 102-102
291-291: _last_active_schedule may reference stale schedule data.This duplicates a previous review comment. The
_last_active_scheduleattribute is set once in__init__(line 128, though not shown in the diff) and opportunistically in thehvac_modesproperty, but it's never updated when the schedule actually changes via other means (e.g., through the thermostat select entity or coordinator updates). Whenset_schedule_state()is called with this value, it may pass an outdated schedule name.Either reference
self.device.get("select_schedule")directly at line 291 to always use current data, or implement proper state tracking that updates_last_active_schedulewhenever the schedule changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/climate.py(6 hunks)tests/components/plugwise/test_climate.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup for HA-core (dev)
- GitHub Check: Setup for HA-core (release/master)
🔇 Additional comments (1)
tests/components/plugwise/test_climate.py (1)
108-108: Test expectations updated correctly, but may need revision.The test assertions correctly reflect the new
set_schedule_statesignature with the addedlast_active_scheduleparameter. However, these tests verify the current implementation which has critical logic errors (always-True condition on line 230 of climate.py).Once the logic errors in the main implementation are addressed, verify that these expected values remain correct, particularly if the schedule values
"GF7 Woonkamer"and"standaard"should be passed or if they might beNonein certain scenarios.Also applies to: 337-337
bouwew
force-pushed
the
save_last_option
branch
from
9f349d6 to
ae135f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/plugwise/climate.py (1)
287-292: HandleNonevalue for_last_active_schedulebefore callingset_schedule_state.The
set_schedule_statemethod signature expects a non-optional string for the schedule name parameter:async def set_schedule_state(self, loc_id: str, name: str, state: str) -> bool. However,_last_active_scheduleis initialized toNoneand only updated when a valid schedule is encountered. If no valid schedule is ever found, passingNonetoset_schedule_statewill cause a type mismatch.Add a guard before the call:
if hvac_mode != HVACMode.OFF and self._last_active_schedule is not None: await self.coordinator.api.set_schedule_state( self._location, STATE_ON if hvac_mode == HVACMode.AUTO else STATE_OFF, self._last_active_schedule, )Alternatively, provide a fallback schedule name if
_last_active_scheduleisNone.
♻️ Duplicate comments (1)
custom_components/plugwise/climate.py (1)
27-27: Remove unusedRestoreEntityinheritance.
RestoreEntityis inherited but noasync_added_to_hassmethod is implemented to restore state. The_last_active_scheduleattribute is populated dynamically from device data in thehvac_modesproperty (lines 229-231), making persistence across restarts unnecessary for the current use case.Since this was previously flagged and marked as addressed but still appears in the code, please verify whether this inheritance is actually needed or if it was reintroduced unintentionally.
Apply this diff to remove the unnecessary inheritance:
-from homeassistant.helpers.restore_state import RestoreEntity-class PlugwiseClimateEntity(PlugwiseEntity, ClimateEntity, RestoreEntity): +class PlugwiseClimateEntity(PlugwiseEntity, ClimateEntity):Also applies to: 102-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/climate.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 GitHub Actions: Test PR against HA-core
custom_components/plugwise/climate.py
[error] 230-230: PLR1714: Consider merging multiple comparisons: schedule not in (NONE, "off"). Use a set if the elements are hashable.
🔇 Additional comments (1)
custom_components/plugwise/climate.py (1)
111-111: LGTM: Attribute declaration is correct.The type hint and initialization are properly formatted.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
custom_components/plugwise/climate.py (3)
144-145: Optimize attribute access after implementingfrom_dict.Once the
from_dictclassmethod is added toPlugwiseClimateExtraStoredData, these lines can access attributes directly instead of callingas_dict()twice.After implementing
from_dict, apply this diff:- self._last_active_schedule = last_extra_data.as_dict()["last_active_schedule"] - self._previous_action_mode = last_extra_data.as_dict()["previous_action_mode"] + self._last_active_schedule = last_extra_data.last_active_schedule + self._previous_action_mode = last_extra_data.previous_action_mode
241-241: Remove or clean up debug logging.This debug log appears to contain temporary debugging text ("HOI") and a typo ("havc_mode" should be "hvac_mode"). Consider removing it or fixing it before merging.
If you want to keep the logging, apply this diff:
- LOGGER.debug("HOI havc_mode = %s", str(hvac)) + LOGGER.debug("HVAC mode: %s", hvac)Or remove it entirely if it was only for debugging:
- - LOGGER.debug("HOI havc_mode = %s", str(hvac)) return hvac
272-286: Consider moving state tracking to coordinator update callback.The
hvac_actionproperty updates_previous_action_modeas a side effect. While property getters with side effects are generally discouraged, this implementation is better than the previous approach (updating in thehvac_modesgetter that was flagged in past reviews).For cleaner architecture, consider tracking mode changes in a
_handle_coordinator_updatecallback override instead of within a property getter. This would separate state tracking from state retrieval.Example approach:
@callback def _handle_coordinator_update(self) -> None: """Handle updated data from the coordinator.""" if ( REGULATION_MODES in self._gateway_data and HVACAction.COOLING in self._gateway_data[REGULATION_MODES] ): mode = self._gateway_data[SELECT_REGULATION_MODE] if mode in (HVACAction.COOLING, HVACAction.HEATING): self._previous_action_mode = mode super()._handle_coordinator_update()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/climate.py(10 hunks)tests/components/plugwise/test_climate.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/components/plugwise/test_climate.py
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🔇 Additional comments (2)
custom_components/plugwise/climate.py (2)
5-5: LGTM! Imports are appropriate for RestoreEntity implementation.The added imports for dataclasses, restore state functionality, and state constants are all properly used in the implementation.
Also applies to: 24-25, 30-30, 44-44
312-343: LGTM! Schedule tracking logic is correctly implemented.The
async_set_hvac_modemethod properly:
- Retrieves and validates the current schedule before storing it
- Updates
_last_active_scheduleonly when a valid schedule exists- Passes the schedule to
set_schedule_statefor persistence- Uses
_previous_action_modeto restore the correct heating/cooling modeThis is the appropriate place to track schedule changes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
custom_components/plugwise/climate.py (3)
127-127: Fix type annotation mismatch.The field is annotated as
strbut initialized withHVACAction.HEATING(an enum), and the dataclass field at line 110 is typed asHVACAction | None. This creates a type mismatch.Apply this diff:
- _previous_action_mode: str = HVACAction.HEATING # Upstream + _previous_action_mode: HVACAction = HVACAction.HEATING # UpstreamBased on learnings
195-198: Add missing argument to constructor.The
PlugwiseClimateExtraStoredDatadataclass has two fields (last_active_scheduleandprevious_action_mode), but onlyself._last_active_scheduleis passed. This meansprevious_action_modewon't be persisted across restarts.Apply this diff:
def extra_restore_state_data(self) -> PlugwiseClimateExtraStoredData: """Return text specific state data to be restored.""" - return PlugwiseClimateExtraStoredData(self._last_active_schedule) + return PlugwiseClimateExtraStoredData( + self._last_active_schedule, self._previous_action_mode + )Based on learnings
105-115: Addfrom_dictclassmethod for proper deserialization.The dataclass implements
as_dict()for serialization but lacks the correspondingfrom_dict()classmethod required by Home Assistant'sRestoreEntity. Without it,async_get_last_extra_data()cannot properly reconstruct the dataclass from stored data, causing restoration to fail.Add the classmethod:
def as_dict(self) -> dict[str, Any]: """Return a dict representation of the text data.""" return asdict(self) + + @classmethod + def from_dict(cls, restored: dict[str, Any]) -> PlugwiseClimateExtraStoredData: + """Initialize a stored data object from a dict.""" + try: + previous_action_mode = ( + HVACAction(restored["previous_action_mode"]) + if restored.get("previous_action_mode") is not None + else None + ) + except (KeyError, ValueError): + previous_action_mode = None + + return cls( + last_active_schedule=restored.get("last_active_schedule"), + previous_action_mode=previous_action_mode, + )Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/climate.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🔇 Additional comments (2)
custom_components/plugwise/climate.py (2)
272-286: LGTM!The logic correctly tracks the previous HVAC action mode when regulation modes are available, and only updates when the mode is
COOLINGorHEATING. This ensures the correct mode is restored when switching from OFF.
312-343: LGTM!The HVAC mode switching logic correctly captures and stores the current schedule when it's valid, and properly restores the previous action mode when switching from OFF. The schedule is passed correctly to
set_schedule_state.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
custom_components/plugwise/climate.py (5)
103-116: Past reviews comprehensively covered dataclass issues.Previous reviews correctly identified that the
from_dictclassmethod is required for proper deserialization by RestoreEntity'sasync_get_last_extra_data(). The review provides a complete implementation example.Based on past reviews.
131-138: Past review correctly identified inefficient attribute access.The previous review accurately flagged that lines 137-138 unnecessarily call
as_dict()twice when the dataclass attributes can be accessed directly. The suggested fix is correct and improves both performance and readability.Based on past reviews.
186-192: Past review correctly identified missing constructor argument.The previous review accurately caught that
previous_action_modeis not being passed to thePlugwiseClimateExtraStoredDataconstructor, which would prevent it from being persisted across restarts. Both fields must be included for complete state restoration.Based on past reviews.
235-235: Past review correctly identified debug code leftover.The previous review accurately flagged this debug statement containing informal text "HOI" and a typo "havc_mode". This should be removed before merging to production.
Based on past reviews.
268-275: State mutation in property getter (already flagged).Past reviews correctly identified that mutating
_previous_action_modeinside thehvac_actionproperty getter violates the principle that properties should be read-only. This property is called frequently by Home Assistant for UI rendering, causing unnecessary state updates.Minor addition: Line 268 comment has a typo - "havc_action" should be "hvac_action".
Based on past reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/climate.py(11 hunks)tests/components/plugwise/test_climate.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
PR: plugwise/plugwise-beta#906
File: tests/components/plugwise/test_binary_sensor.py:9-16
Timestamp: 2025-08-23T16:26:34.700Z
Learning: The Plugwise integration test files import utilities like `snapshot_platform`, `MockConfigEntry`, and `async_fire_time_changed` from `tests.common`, which is from the Home Assistant core repository at https://github.com/home-assistant/core/blob/master/tests/common.py, not from local test files.
Applied to files:
tests/components/plugwise/test_climate.py
🧬 Code graph analysis (2)
tests/components/plugwise/test_climate.py (2)
custom_components/plugwise/climate.py (2)
PlugwiseClimateExtraStoredData(104-115)as_dict(110-115)tests/components/plugwise/conftest.py (2)
mock_smile_adam_heat_cool(185-208)init_integration(347-356)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🔇 Additional comments (4)
tests/components/plugwise/test_climate.py (3)
25-25: LGTM! Imports are correctly added for restore state functionality.The new imports support the restore state test:
PlugwiseClimateExtraStoredDatafrom the climate module,Statefor creating mock state objects, andmock_restore_cache_with_extra_datafor mocking the restore cache.Also applies to: 27-27, 33-38
115-115: LGTM! Additional parameter aligns with implementation changes.The
set_schedule_statecalls now correctly pass the schedule/location name as a third parameter, matching the updated implementation that propagatesschedulefromdevice.get("select_schedule").Also applies to: 367-367
204-204: LGTM! Test data updates support HVAC mode change scenarios.The
climate_modefield updates to"heat"and"cool"correctly simulate regulation mode changes in the test data, allowing proper verification of the climate entity's behavior when switching between heating and cooling modes.Also applies to: 225-225
custom_components/plugwise/climate.py (1)
127-128: Attribute initialization looks correct.The
_last_active_scheduleand_previous_action_modeattributes are properly initialized with appropriate types and default values for tracking schedule and regulation mode state.
bouwew
force-pushed
the
save_last_option
branch
from
1103f0c to
542f85f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
custom_components/plugwise/climate.py (3)
146-147: Access dataclass attributes directly instead of callingas_dict()twice.Calling
as_dict()twice is inefficient when you can access the dataclass attributes directly.Apply this diff:
- self._last_active_schedule = extra_data.as_dict()["last_active_schedule"] - self._previous_action_mode = extra_data.as_dict()["previous_action_mode"] + self._last_active_schedule = extra_data.last_active_schedule + self._previous_action_mode = extra_data.previous_action_mode
244-244: Remove debug logging leftover.This debug statement contains informal text ("HOI") and a typo ("havc_mode" should be "hvac_mode"). It appears to be leftover debug code.
Remove the debug statement:
- LOGGER.debug("HOI havc_mode = %s", str(hvac))
321-333: Critical: Stored schedule is never actually used.The PR objective states: "storing the last_used_schedule so it can be reused when changing HVAC mode from heat to auto." However, the current implementation retrieves the current device schedule and always passes that to
set_schedule_state, never actually using the stored_last_active_schedulevalue. This defeats the purpose of the restoration logic.Current flow:
- Line 321: Gets current device schedule
- Lines 322-327: Saves to
_last_active_scheduleif None- Line 332: Passes the current device
schedule(not_last_active_schedule)Expected flow for the stated objective:
- When switching to AUTO, prefer the stored
_last_active_scheduleif available- Only fall back to current device schedule if no stored schedule exists
- Save valid schedules when they're active for future reuse
Consider revising the logic:
if hvac_mode != HVACMode.OFF: - schedule = self.device.get("select_schedule") - if self._last_active_schedule is None: - if schedule is not None: - if schedule != "off": - self._last_active_schedule = schedule - else: - raise HomeAssistantError("Failed setting HVACMode, set a schedule first") + # When switching to AUTO, use stored schedule if available + if hvac_mode == HVACMode.AUTO and self._last_active_schedule: + schedule = self._last_active_schedule + else: + schedule = self.device.get("select_schedule") + # Save valid schedules for future reuse + if schedule is not None and schedule != "off": + self._last_active_schedule = schedule + elif self._last_active_schedule is None: + raise HomeAssistantError("Failed setting HVACMode, set a schedule first") await self.coordinator.api.set_schedule_state( self._location, STATE_ON if hvac_mode == HVACMode.AUTO else STATE_OFF, schedule, )
🧹 Nitpick comments (1)
custom_components/plugwise/climate.py (1)
327-327: Consider extracting exception message to a constant.Static analysis suggests avoiding long messages directly in exception constructors. While this is a minor style issue, defining error messages as module-level constants improves maintainability and makes them easier to test.
For example, define at module level:
ERROR_NO_SCHEDULE = "Failed setting HVACMode, set a schedule first"Then use:
- raise HomeAssistantError("Failed setting HVACMode, set a schedule first") + raise HomeAssistantError(ERROR_NO_SCHEDULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/climate.py(10 hunks)tests/components/plugwise/test_climate.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
PR: plugwise/plugwise-beta#906
File: tests/components/plugwise/test_binary_sensor.py:9-16
Timestamp: 2025-08-23T16:26:34.700Z
Learning: The Plugwise integration test files import utilities like `snapshot_platform`, `MockConfigEntry`, and `async_fire_time_changed` from `tests.common`, which is from the Home Assistant core repository at https://github.com/home-assistant/core/blob/master/tests/common.py, not from local test files.
Applied to files:
tests/components/plugwise/test_climate.py
🧬 Code graph analysis (2)
tests/components/plugwise/test_climate.py (2)
custom_components/plugwise/climate.py (2)
PlugwiseClimateExtraStoredData(105-124)as_dict(111-116)tests/components/plugwise/conftest.py (2)
mock_smile_adam_heat_cool(185-208)init_integration(347-356)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 GitHub Actions: Test PR against HA-core
tests/components/plugwise/test_climate.py
[error] 177-177: Ruff: Undefined name 'ANY'.
[error] 178-178: Ruff: Undefined name 'STATE_ON'.
🪛 Ruff (0.14.1)
tests/components/plugwise/test_climate.py
178-178: Undefined name ANY
(F821)
178-178: Undefined name STATE_ON
(F821)
custom_components/plugwise/climate.py
327-327: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
tests/components/plugwise/test_climate.py (1)
169-179: Test may fail due to implementation logic issue.This test verifies that the restored schedule "Weekschema" is used when switching to AUTO mode (line 178). However, based on the implementation in
climate.pylines 321-332, the current code always uses the device's current schedule rather than the stored_last_active_schedule. This means the test assertion may fail unless the implementation is fixed to actually use the restored schedule value when switching to AUTO mode.After fixing the imports, verify this test passes. If it fails, it confirms the implementation issue flagged in the
climate.pyreview where the stored schedule is never actually used.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/components/plugwise/test_climate.py (3)
115-116: Prefer STATE_OFF over HVACMode.OFF in assertion.The implementation calls
set_schedule_state(..., STATE_OFF, ...). Use the constant for tighter matching.- mock_smile_adam.set_schedule_state.assert_called_with( - "c50f167537524366a5af7aa3942feb1e", HVACMode.OFF, "GF7 Woonkamer", - ) + mock_smile_adam.set_schedule_state.assert_called_with( + "c50f167537524366a5af7aa3942feb1e", STATE_OFF, "GF7 Woonkamer", + )Add import if needed:
-from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATURE +from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATURE, STATE_OFF
147-185: Use STATE_ON constant in restore test for consistency.The integration passes
STATE_ON/STATE_OFF; avoid raw"on".- mock_smile_adam_heat_cool.set_schedule_state.assert_called_with( - "f2bf9048bef64cc5b6d5110154e33c81", "on", "Weekschema" - ) + mock_smile_adam_heat_cool.set_schedule_state.assert_called_with( + "f2bf9048bef64cc5b6d5110154e33c81", STATE_ON, "Weekschema" + )Also add import:
-from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATURE +from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATURE, STATE_ON
385-386: Prefer STATE_OFF over HVACMode.OFF in assertion.- mock_smile_anna.set_schedule_state.assert_called_with( - "c784ee9fdab44e1395b8dee7d7a497d5", HVACMode.OFF, "standaard", - ) + mock_smile_anna.set_schedule_state.assert_called_with( + "c784ee9fdab44e1395b8dee7d7a497d5", STATE_OFF, "standaard", + )Add import if needed:
-from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATURE +from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATURE, STATE_OFFcustom_components/plugwise/climate.py (4)
135-137: Annotate_previous_action_modeto the enum type.Improves clarity and linting.
- _previous_action_mode = HVACAction.HEATING # Upstream + _previous_action_mode: HVACAction = HVACAction.HEATING # Upstream
139-147: Use dataclass attributes on restore; avoid dict round‑trip.Also ensure the restored enum is of type
HVACAction.if (extra_data := await self.async_get_last_extra_data()): LOGGER.debug("Extra data: %s", extra_data) - self._last_active_schedule = extra_data.as_dict()["last_active_schedule"] - self._previous_action_mode = extra_data.as_dict()["previous_action_mode"] + self._last_active_schedule = extra_data.last_active_schedule + self._previous_action_mode = ( + extra_data.previous_action_mode + if isinstance(extra_data.previous_action_mode, HVACAction) + else HVACAction(extra_data.previous_action_mode) + if extra_data.previous_action_mode + else self._previous_action_mode + )
243-243: Remove stray debug log and typo.Leftover debug with “HOI”/“havc_mode”.
- LOGGER.debug("HOI havc_mode = %s", str(hvac))
276-284: Avoid side effects in property getter.Updating
_previous_action_modeinsidehvac_actionmixes read with write. Move this into a coordinator update hook or where regulation mode changes are processed.Would you like a patch that updates
_previous_action_modein a@callback _handle_coordinator_update?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/climate.py(9 hunks)tests/components/plugwise/test_climate.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
PR: plugwise/plugwise-beta#906
File: tests/components/plugwise/test_binary_sensor.py:9-16
Timestamp: 2025-08-23T16:26:34.700Z
Learning: The Plugwise integration test files import utilities like `snapshot_platform`, `MockConfigEntry`, and `async_fire_time_changed` from `tests.common`, which is from the Home Assistant core repository at https://github.com/home-assistant/core/blob/master/tests/common.py, not from local test files.
Applied to files:
tests/components/plugwise/test_climate.py
🧬 Code graph analysis (2)
tests/components/plugwise/test_climate.py (2)
custom_components/plugwise/climate.py (2)
PlugwiseClimateExtraStoredData(104-123)as_dict(110-115)tests/components/plugwise/conftest.py (2)
mock_smile_adam_heat_cool(185-208)mock_config_entry(81-95)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 Ruff (0.14.1)
custom_components/plugwise/climate.py
328-328: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
tests/components/plugwise/test_climate.py (4)
33-38: LGTM: correct test utilities imported.
222-223: LGTM: climate_mode test data aligned with enums.
243-244: LGTM: cooling branch test data aligned.
25-25: ****The import path in the test file is correct. In Home Assistant custom component test infrastructure, custom components located in
custom_components/plugwise/are automatically available under thehomeassistant.components.plugwisenamespace during testing. This is the standard pattern used in the test fixtures themselves (seeconftest.pywhich also imports fromhomeassistant.components.plugwise). Changing the import tocustom_components.plugwise.climatewould break the tests.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/components/plugwise/test_climate.py (1)
146-184: Good test coverage for restoration, but consider improvements.The test correctly addresses the past review feedback by using
HVACAction.HEATINGat line 160. However, consider these improvements:
Line 182: Use
STATE_ONconstant for consistency. Replace the string"on"with theSTATE_ONconstant (which would need to be imported fromhomeassistant.const). This ensures consistency with the codebase and prevents issues if the string representation changes.Test verification is limited to external behavior. The test only verifies that
set_schedule_statewas called correctly, but doesn't confirm that the internal state variables (_last_active_schedule,_previous_action_mode) were actually restored. Consider accessing the climate entity's attributes or internal state to verify restoration happened correctly.Apply this diff for the constant:
+from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATURE, STATE_ON -from homeassistant.const import ATTR_ENTITY_ID, ATTR_TEMPERATUREAnd update the assertion:
mock_smile_adam_heat_cool.set_schedule_state.assert_called_with( - "f2bf9048bef64cc5b6d5110154e33c81", "on", "Weekschema" + "f2bf9048bef64cc5b6d5110154e33c81", STATE_ON, "Weekschema" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/components/plugwise/test_climate.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
PR: plugwise/plugwise-beta#906
File: tests/components/plugwise/test_binary_sensor.py:9-16
Timestamp: 2025-08-23T16:26:34.700Z
Learning: The Plugwise integration test files import utilities like `snapshot_platform`, `MockConfigEntry`, and `async_fire_time_changed` from `tests.common`, which is from the Home Assistant core repository at https://github.com/home-assistant/core/blob/master/tests/common.py, not from local test files.
Applied to files:
tests/components/plugwise/test_climate.py
🧬 Code graph analysis (1)
tests/components/plugwise/test_climate.py (2)
custom_components/plugwise/climate.py (2)
PlugwiseClimateExtraStoredData(104-123)as_dict(110-115)tests/components/plugwise/conftest.py (2)
mock_smile_adam_heat_cool(185-208)mock_config_entry(81-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Setup for HA-core (dev)
🔇 Additional comments (5)
tests/components/plugwise/test_climate.py (5)
25-37: LGTM! Imports are appropriate for the new restoration test.The new imports support the restore_state testing functionality correctly.
221-221: LGTM! Test data correctly updated for climate_mode field.The addition of the
climate_modefield aligns with the heating state being tested.
242-242: LGTM! Test data correctly updated for climate_mode field.The addition of the
climate_modefield aligns with the cooling state being tested.
114-114: LGTM! Mock call updated for new API signature.The additional location parameter
"GF7 Woonkamer"correctly reflects the updatedset_schedule_statesignature.
384-384: LGTM! Mock call updated for new API signature.The additional schedule name parameter
"standaard"correctly reflects the updatedset_schedule_statesignature.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
custom_components/plugwise/climate.py (3)
103-124: Serialize enum to string value for JSON compatibility.The
previous_action_modefield stores anHVACActionenum directly, but Home Assistant's state restoration expects JSON-serializable data. Enums are not JSON-safe and may cause serialization errors or type instability across restarts.Apply this diff to serialize the enum as a string in
as_dict()and rehydrate it infrom_dict():def as_dict(self) -> dict[str, Any]: """Return a dict representation of the text data.""" return { "last_active_schedule": self.last_active_schedule, - "previous_action_mode": self.previous_action_mode, + "previous_action_mode": ( + self.previous_action_mode.value if self.previous_action_mode else None + ), } @classmethod def from_dict(cls, restored: dict[str, Any]) -> PlugwiseClimateExtraStoredData: """Initialize a stored data object from a dict.""" + prev = restored.get("previous_action_mode") return cls( last_active_schedule=restored.get("last_active_schedule"), - previous_action_mode=restored.get("previous_action_mode"), + previous_action_mode=HVACAction(prev) if prev else None, )
145-146: Access dataclass attributes directly instead of callingas_dict()twice.These lines inefficiently call
as_dict()twice to extract values from aPlugwiseClimateExtraStoredDatainstance. Sinceextra_datais already a dataclass instance, access its attributes directly for better performance and clarity.Apply this diff:
- self._last_active_schedule = extra_data.as_dict()["last_active_schedule"] - self._previous_action_mode = extra_data.as_dict()["previous_action_mode"] + self._last_active_schedule = extra_data.last_active_schedule + self._previous_action_mode = extra_data.previous_action_mode
242-244: Remove leftover debug statement.Line 243 contains informal text ("HOI") and a typo ("havc_mode" instead of "hvac_mode"). This appears to be debug code that should be removed before merging.
Apply this diff:
- - LOGGER.debug("HOI havc_mode = %s", str(hvac)) return hvac
🧹 Nitpick comments (1)
custom_components/plugwise/climate.py (1)
136-136: Add explicit type annotation.Line 136 assigns
HVACAction.HEATINGbut lacks a type annotation, unlike line 135. For consistency and type safety, add an explicit annotation.Apply this diff:
- _previous_action_mode = HVACAction.HEATING # Upstream + _previous_action_mode: HVACAction = HVACAction.HEATING # Upstream
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/climate.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 Ruff (0.14.1)
custom_components/plugwise/climate.py
331-333: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
custom_components/plugwise/climate.py (4)
126-126: LGTM!
RestoreEntityinheritance is now properly implemented with the requiredasync_added_to_hassandextra_restore_state_datamethods.
194-200: LGTM!The property correctly returns a
PlugwiseClimateExtraStoredDatainstance with both fields, ensuring proper state persistence across restarts.
274-288: LGTM!The property correctly tracks
_previous_action_modewhen regulation modes are available and cooling/heating is active. This enables proper restoration when switching HVAC modes back on.
319-339: Schedule selection logic correctly implements PR objectives.The implementation now properly stores and reuses
_last_active_schedulewhen switching HVAC modes:
- Captures the last valid schedule when it's not "off"
- Reuses the saved schedule when the current schedule is "off"
- Raises an error if attempting to enable AUTO mode without a schedule
Optional: Static analysis flags TRY003 for the error message on line 332. Consider extracting it to a module-level constant if preferred:
# At module level _NO_SCHEDULE_ERROR = "Failed setting HVACMode, set a schedule first" # In the method raise HomeAssistantError(_NO_SCHEDULE_ERROR)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
custom_components/plugwise/climate.py (2)
103-123: Fix enum serialization and type annotation.Two issues with the dataclass:
Type mismatch: Line 108 declares
previous_action_mode: HVACAction(non-optional), butfrom_dictat line 122 usesrestored.get("previous_action_mode")which returnsNonefor missing keys. This creates a type inconsistency.Enum serialization: Lines 114 and 122 store and retrieve the
HVACActionenum directly. Home Assistant's state restoration serializes to JSON, which doesn't natively support enum objects. You must serialize to the enum's string value and deserialize back to the enum.Apply this diff:
@dataclass class PlugwiseClimateExtraStoredData(ExtraStoredData): """Object to hold extra stored data.""" last_active_schedule: str | None - previous_action_mode: HVACAction + previous_action_mode: HVACAction | None def as_dict(self) -> dict[str, Any]: """Return a dict representation of the text data.""" return { "last_active_schedule": self.last_active_schedule, - "previous_action_mode": self.previous_action_mode, + "previous_action_mode": ( + self.previous_action_mode.value if self.previous_action_mode else None + ), } @classmethod def from_dict(cls, restored: dict[str, Any]) -> PlugwiseClimateExtraStoredData: """Initialize a stored data object from a dict.""" + prev = restored.get("previous_action_mode") return cls( last_active_schedule=restored.get("last_active_schedule"), - previous_action_mode=restored.get("previous_action_mode"), + previous_action_mode=HVACAction(prev) if prev else None, )
145-146: Critical syntax error:as_dictis a method, not a property.Lines 145-146 access
as_dictwithout parentheses, treating it as a property when it's actually a method defined at line 110. This will raise anAttributeErrorat runtime when Home Assistant attempts to restore state.Apply this diff to access the dataclass attributes directly:
if (extra_data := await self.async_get_last_extra_data()): LOGGER.debug("Extra data: %s", extra_data) - self._last_active_schedule = extra_data.as_dict["last_active_schedule"] - self._previous_action_mode = extra_data.as_dict["previous_action_mode"] + self._last_active_schedule = extra_data.last_active_schedule + self._previous_action_mode = extra_data.previous_action_mode
🧹 Nitpick comments (1)
custom_components/plugwise/climate.py (1)
135-136: Add type annotation to_previous_action_mode.Line 136 initializes
_previous_action_modewithout a type annotation. For consistency with line 135 and to match the dataclass field type, add an explicit annotation.Apply this diff:
_last_active_schedule: str | None = None - _previous_action_mode = HVACAction.HEATING # Upstream + _previous_action_mode: HVACAction = HVACAction.HEATING # UpstreamIf
_previous_action_modecan beNoneafter restoration (when extra data is missing or invalid), consider usingHVACAction | Noneinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/climate.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 Ruff (0.14.1)
custom_components/plugwise/climate.py
330-332: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup for HA-core (dev)
- GitHub Check: Setup for HA-core (release/master)
🔇 Additional comments (5)
custom_components/plugwise/climate.py (5)
194-200: LGTM!The property correctly returns both fields for state restoration using clear keyword arguments.
275-282: LGTM!The logic correctly tracks the previous action mode when the gateway supports both cooling and heating, preserving it for later restoration when switching from OFF mode.
346-346: LGTM!Correctly uses the tracked
_previous_action_modewhen restoring regulation mode after exiting OFF state.
5-5: LGTM!The new imports are necessary and properly used:
dataclassforPlugwiseClimateExtraStoredDataHomeAssistantErrorfor validation errorsExtraStoredData, RestoreEntityfor state persistenceAlso applies to: 27-27, 29-29
318-338: Address potential None schedule edge case and TRY003 linter warning.The code initializes
_last_active_scheduleasNone(line 135) and only validates schedules when enabling AUTO mode (lines 329-332). However, when switching to non-AUTO modes like HEAT or OFF, if_last_active_scheduleremainsNone(e.g., on first boot before any schedule is set), the code callsset_schedule_statewith aNoneschedule parameter even though tests don't cover this edge case.Two recommendations:
Verify API contract: Confirm that the Plugwise API's
set_schedule_statemethod acceptsNoneschedules when the state is OFF. If not, add similar validation for non-AUTO modes or handle the None case explicitly.Address TRY003 linter warning: Move the error message to a module-level constant to resolve the linter flag:
+_NO_SCHEDULE_ERROR = "Failed setting HVACMode, set a schedule first" + ... if hvac_mode == HVACMode.AUTO and not desired: - raise HomeAssistantError( - "Failed setting HVACMode, set a schedule first" - ) + raise HomeAssistantError(_NO_SCHEDULE_ERROR)
bouwew
force-pushed
the
save_last_option
branch
from
c1574ac to
83892ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
custom_components/plugwise/climate.py (1)
103-123: Critical bug infrom_dictand missing enum serialization.Three issues in the dataclass:
Critical: Line 122 accesses
restored.previous_action_modeas an attribute, butrestoredis adict[str, Any]. This will raiseAttributeErrorat runtime.Major: The enum is not serialized to a string in
as_dict(). Home Assistant's state restoration requires JSON-serializable data. Storing the enum object directly (line 114) will fail or cause type issues on deserialization.Minor: Line 108 has a trailing space after the type annotation.
Apply this diff to fix all three issues:
@dataclass class PlugwiseClimateExtraStoredData(ExtraStoredData): """Object to hold extra stored data.""" last_active_schedule: str | None - previous_action_mode: HVACAction + previous_action_mode: HVACAction | None def as_dict(self) -> dict[str, Any]: """Return a dict representation of the text data.""" return { "last_active_schedule": self.last_active_schedule, - "previous_action_mode": self.previous_action_mode, + "previous_action_mode": ( + self.previous_action_mode.value if self.previous_action_mode else None + ), } @classmethod def from_dict(cls, restored: dict[str, Any]) -> PlugwiseClimateExtraStoredData: """Initialize a stored data object from a dict.""" + prev = restored.get("previous_action_mode") return cls( last_active_schedule=restored.get("last_active_schedule"), - previous_action_mode=HVACAction(restored.previous_action_mode), + previous_action_mode=HVACAction(prev) if prev else None, )
🧹 Nitpick comments (1)
custom_components/plugwise/climate.py (1)
145-146: Avoid redundantas_dict()calls.These lines call
as_dict()to access dictionary keys, butextra_datais already aPlugwiseClimateExtraStoredDatainstance. Access its attributes directly for better performance and clarity.Apply this diff:
if (extra_data := await self.async_get_last_extra_data()): LOGGER.debug("Extra data: %s", extra_data) - self._last_active_schedule = extra_data.as_dict()["last_active_schedule"] - self._previous_action_mode = extra_data.as_dict()["previous_action_mode"] + self._last_active_schedule = extra_data.last_active_schedule + self._previous_action_mode = extra_data.previous_action_mode
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/climate.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 GitHub Actions: Test PR against HA-core
custom_components/plugwise/climate.py
[error] 105-105: Ruff formatting/linting issues were automatically fixed by the pre-commit hook. The hook reported: 'Found 1 error (1 fixed, 0 remaining)'. Command: 'pre-commit run --all-files --show-diff-on-failure'.
🪛 Ruff (0.14.1)
custom_components/plugwise/climate.py
330-332: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
custom_components/plugwise/climate.py (3)
318-338: Verifyset_schedule_statehandlesNoneschedule withSTATE_OFF.When switching to a non-AUTO mode (e.g., HEAT) and the current schedule is "off" with no saved
_last_active_schedule, the variabledesiredbecomesNone(line 326). ThisNoneis then passed toset_schedule_statewithSTATE_OFF(lines 334-338).Confirm that the API method tolerates
schedule=Nonewhenstate=STATE_OFF, or add a fallback to prevent passingNone:# Enabling HVACMode.AUTO requires a previously set schedule if hvac_mode == HVACMode.AUTO and not desired: raise HomeAssistantError( "Failed setting HVACMode, set a schedule first" ) # Fallback for non-AUTO modes when no valid schedule available if not desired and hvac_mode != HVACMode.AUTO: return # or handle appropriately based on API requirementsAlso consider shortening the error message or moving it to a module constant to address the TRY003 linter hint.
194-200: LGTM: State data properly packaged for restoration.The property correctly returns both fields for persistence. Once the dataclass serialization issues are fixed, this will work as intended.
273-287: LGTM: Previous action mode tracking implemented correctly.The logic properly tracks
_previous_action_modewhen the gateway supports both heating and cooling, allowing the system to restore the appropriate regulation mode when switching back from OFF.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
custom_components/plugwise/climate.py (1)
334-336: Consider shortening or extracting the error message.The linter flags this error message as too long per TRY003. While the message is clear, you could improve maintainability by extracting it to a module-level constant or shortening it slightly.
For example:
# At module level _ERR_NO_SCHEDULE = "Set a schedule before enabling Auto" # In the method if hvac_mode == HVACMode.AUTO and not desired: raise HomeAssistantError(_ERR_NO_SCHEDULE)Or simply shorten inline:
- raise HomeAssistantError( - "Failed setting HVACMode, set a schedule first" - ) + raise HomeAssistantError("Set a schedule before enabling Auto")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/climate.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
🪛 Ruff (0.14.1)
custom_components/plugwise/climate.py
334-336: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Prepare and validate pre-commit
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
custom_components/plugwise/climate.py (1)
321-341: LGTM! Schedule handling correctly implements restoration logic.The implementation properly:
- Captures the current schedule when valid
- Reuses the saved schedule when the current is "off"
- Raises an error when switching to AUTO without a valid schedule
- Passes the appropriate schedule to
set_schedule_stateThis fulfills the PR objective of storing and reusing the last schedule when switching HVAC modes.
Optional: Address static analysis hint.
The linter suggests (TRY003) keeping error messages shorter or defining them as module constants. This is a minor style issue.
If desired, apply this diff:
+# Error message constants +ERROR_NO_SCHEDULE = "Failed setting HVACMode, set a schedule first" + # ... rest of code ... # Enabling HVACMode.AUTO requires a previously set schedule if hvac_mode == HVACMode.AUTO and not desired: - raise HomeAssistantError( - "Failed setting HVACMode, set a schedule first" - ) + raise HomeAssistantError(ERROR_NO_SCHEDULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
custom_components/plugwise/climate.py(8 hunks)custom_components/plugwise/manifest.json(1 hunks)tests/components/plugwise/test_climate.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
PR: plugwise/plugwise-beta#906
File: tests/components/plugwise/test_binary_sensor.py:9-16
Timestamp: 2025-08-23T16:26:34.700Z
Learning: The Plugwise integration test files import utilities like `snapshot_platform`, `MockConfigEntry`, and `async_fire_time_changed` from `tests.common`, which is from the Home Assistant core repository at https://github.com/home-assistant/core/blob/master/tests/common.py, not from local test files.
Applied to files:
tests/components/plugwise/test_climate.py
🧬 Code graph analysis (2)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
tests/components/plugwise/test_climate.py (2)
custom_components/plugwise/climate.py (2)
PlugwiseClimateExtraStoredData(104-123)as_dict(110-115)tests/components/plugwise/conftest.py (2)
mock_smile_adam_heat_cool(185-208)mock_config_entry(81-95)
🪛 Ruff (0.14.1)
custom_components/plugwise/climate.py
333-335: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Setup for HA-core (dev)
🔇 Additional comments (7)
custom_components/plugwise/manifest.json (1)
10-10: LGTM! Dependency version updated appropriately.The version bump to 1.8.3 aligns with the restoration features introduced in the climate component.
tests/components/plugwise/test_climate.py (3)
25-25: LGTM! Imports correctly support restoration testing.The new imports enable testing of the restoration flow for climate entities.
Also applies to: 27-27, 32-37
146-225: LGTM! Comprehensive restoration test.The test effectively covers:
- Restoration with
Nonevalues (living_room entity)- Error handling when switching to AUTO without a saved schedule
- Successful restoration and reuse of saved schedule (bathroom entity)
- Verification that
set_schedule_statereceives the correct restored schedule
114-114: LGTM! Test updates align with implementation changes.The additional arguments to
set_schedule_stateandclimate_modedata additions correctly reflect the updated API signature and restoration flow.Also applies to: 262-262, 283-283, 425-425
custom_components/plugwise/climate.py (3)
103-123: LGTM! Dataclass correctly implements serialization.The dataclass properly defines both fields as
str | None, and the serialization/deserialization methods correctly handle string values. This aligns with the usage throughout the code where_previous_action_modestores string values likeHVACAction.HEATING.value.
139-149: LGTM! Restoration properly implemented.The
async_added_to_hassmethod correctly:
- Calls the parent method first
- Retrieves and deserializes the extra data
- Restores both
_last_active_scheduleand_previous_action_modeThe
extra_restore_state_dataproperty correctly returns both fields for persistence.Also applies to: 197-203
348-349: LGTM! Correctly restores regulation mode.The code properly uses
_previous_action_modeto restore the correct regulation mode (heating or cooling) when switching from OFF to another HVAC mode.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/climate.py(10 hunks)tests/components/plugwise/test_climate.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
PR: plugwise/plugwise-beta#906
File: tests/components/plugwise/test_binary_sensor.py:9-16
Timestamp: 2025-08-23T16:26:34.700Z
Learning: The Plugwise integration test files import utilities like `snapshot_platform`, `MockConfigEntry`, and `async_fire_time_changed` from `tests.common`, which is from the Home Assistant core repository at https://github.com/home-assistant/core/blob/master/tests/common.py, not from local test files.
Applied to files:
tests/components/plugwise/test_climate.py
🧬 Code graph analysis (2)
custom_components/plugwise/climate.py (1)
custom_components/plugwise/entity.py (2)
PlugwiseEntity(31-93)device(91-93)
tests/components/plugwise/test_climate.py (2)
custom_components/plugwise/climate.py (2)
PlugwiseClimateExtraStoredData(105-124)as_dict(111-116)tests/components/plugwise/conftest.py (2)
mock_smile_adam_heat_cool(185-208)mock_config_entry(81-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup for HA-core (dev)
- GitHub Check: Setup for HA-core (release/master)
🔇 Additional comments (3)
tests/components/plugwise/test_climate.py (3)
25-27: LGTM - Imports support restoration testing.The new imports for
PlugwiseClimateExtraStoredDataandStateare correctly sourced and necessary for testing the restoration functionality added in this PR.
32-37: LGTM - Test utilities properly imported.The addition of
mock_restore_cache_with_extra_datais necessary for the new restoration test and is correctly imported fromtests.common.Based on learnings
262-262: LGTM - Mock data updated to match implementation expectations.The additions of
climate_modeto the mock data ensure test data consistency with the implementation's use ofdevice.get(CLIMATE_MODE)for determining HVAC modes.Also applies to: 283-283
bouwew
force-pushed
the
save_last_option
branch
2 times, most recently
from
0a43fab to
55fcb61
Compare
bouwew
force-pushed
the
save_last_option
branch
from
effb335 to
25347ea
Compare
|
2 participants
This should help setting the correct schedule when changing hvac_mode heat to auto. A prerequisite is that a schedule has been set before disabling it.
Summary by CodeRabbit