Fixing Core Issue #171955#1073
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 32 minutes and 54 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR refactors the Plugwise climate entity's stored-state restoration and HVAC mode control. The ChangesClimate entity restoration and HVAC control
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (1)
custom_components/plugwise/climate.py (1)
165-170: 💤 Low valueCache
as_dict()result and consider defensive access.
extra_data.as_dict()is called twice unnecessarily. Caching the result improves efficiency and using.get()adds defensive handling for potentially malformed stored data.♻️ Suggested refactor
extra_data = await self.async_get_last_extra_data() if extra_data is not None: - self._last_active_schedule = extra_data.as_dict()["last_active_schedule"] - self._previous_action_mode = ( - extra_data.as_dict()["previous_action_mode"] or HVACAction.HEATING.value - ) + data = extra_data.as_dict() + self._last_active_schedule = data.get("last_active_schedule") + self._previous_action_mode = ( + data.get("previous_action_mode") or HVACAction.HEATING.value + )🤖 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 `@custom_components/plugwise/climate.py` around lines 165 - 170, The code calls extra_data.as_dict() twice and doesn't defensively access keys; modify the block that awaits async_get_last_extra_data() to first assign extra = extra_data.as_dict() once, then set self._last_active_schedule = extra.get("last_active_schedule") and self._previous_action_mode = extra.get("previous_action_mode") or HVACAction.HEATING.value so you cache the dict and use .get() with the existing HVACAction.HEATING.value fallback for malformed or missing data.
🤖 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 `@custom_components/plugwise/climate.py`:
- Around line 330-342: The transition logic wrongly falls through to the
"transition to AUTO" schedule-on block for HEAT↔COOL changes; add an explicit
branch before the schedule-on code that detects manual regulation mode switches
(check self.hvac_mode vs previous/desired mode and membership in
REGULATION_MODES and presence of cooling) and call await
self._api.set_regulation_mode(self._location, <desired_mode>) then return,
leaving the existing set_schedule_state(self._location, STATE_ON,
self._last_active_schedule) only for HVACMode.AUTO transitions and keeping the
HomeAssistantError for missing _last_active_schedule unchanged.
- Around line 286-292: The _regulation_mode_for_hvac function can raise
UnboundLocalError because it uses two separate ifs and may leave mode undefined
for unexpected HVACMode values; change the branching to an if/elif (check
hvac_mode == HVACMode.HEAT then elif hvac_mode == HVACMode.COOL) and add an
explicit fallback (raise a ValueError or return a safe default) so that
_regulation_mode_for_hvac, HVACMode and HVACAction usage is deterministic and
fails fast for unsupported modes.
---
Nitpick comments:
In `@custom_components/plugwise/climate.py`:
- Around line 165-170: The code calls extra_data.as_dict() twice and doesn't
defensively access keys; modify the block that awaits
async_get_last_extra_data() to first assign extra = extra_data.as_dict() once,
then set self._last_active_schedule = extra.get("last_active_schedule") and
self._previous_action_mode = extra.get("previous_action_mode") or
HVACAction.HEATING.value so you cache the dict and use .get() with the existing
HVACAction.HEATING.value fallback for malformed or missing data.
🪄 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: 269b3e71-9fd1-4561-a5c5-d9a03cec384b
📒 Files selected for processing (2)
custom_components/plugwise/climate.pytests/components/plugwise/test_climate.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Dev shows linting errors, let's not fix them as part of the core issue this is because the changes in ruff and lint (https://github.com/plugwise/plugwise-beta/actions/runs/26361229109/job/77597017252?pr=1073#step:4:5986) |
|
Confirmed working here (no error, green checkmark, changing hvac_mode) |
bouwew
added
the
needs_upstreaming
There was a problem hiding this comment.
The multiple if statements should be cleaned up, but lets leave that for another time, just to mark for future optimisation
There was a problem hiding this comment.
Pull request overview
This pull request targets Home Assistant Core Issue #171955 by refactoring Plugwise climate state restoration and HVAC mode switching, and by adding/adjusting tests and fixtures to validate the new behavior.
Changes:
- Refactors
PlugwiseClimateEntityrestore handling and HVAC mode transition logic (notably around OFF/AUTO/manual transitions). - Adds tests and a new fixture to cover regulation-off mode transitions and additional schedule/HVAC behavior.
- Bumps package version/changelog and updates CI/cache and local core-testing script behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
custom_components/plugwise/climate.py |
Refactors restore + HVAC mode transition logic for Plugwise climate entities. |
tests/components/plugwise/test_climate.py |
Adds/extends tests around regulation-off and schedule/HVAC transitions. |
tests/components/plugwise/fixtures/m_adam_heating_off_schedule/data.json |
Adds a fixture representing Adam in regulation-off with schedule state. |
scripts/core-testing.sh |
Updates HA-core clone validation to check requirements_test.txt. |
pyproject.toml |
Bumps project version to 0.64.2. |
CHANGELOG.md |
Adds v0.64.2 entry mentioning the Core issue fix and related PRs. |
.github/workflows/test.yml |
Bumps cache version to invalidate prior caches. |
.github/workflows/core_next.yml |
Bumps cache version to invalidate prior caches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Summary by CodeRabbit
Refactor
Tests