You can now set activation hours from service call#32
Merged
jyourstone merged 2 commits intomainfrom Mar 21, 2026
Merged
Conversation
WalkthroughAdds a runtime "Dynamic hours override" to the power_saver integration. Introduces two services — Suggested labels
🚥 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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![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
🧹 Nitpick comments (4)
custom_components/power_saver/__init__.py (1)
89-100: Consider wrapping_find_coordinatorin try/except for better error messages.The service handlers call
_find_coordinatorwhich raisesValueErroron invalid device IDs. While this will fail the service call (correct behavior), wrapping it in a try/except and raisingHomeAssistantErrorwould provide a cleaner error to users via the UI.♻️ Optional improvement for better UX
+from homeassistant.exceptions import HomeAssistantError + async def handle_set_schedule_hours(call: ServiceCall) -> None: """Handle the set_schedule_hours service call.""" device_id = call.data[ATTR_DEVICE_ID] hours = call.data[ATTR_HOURS] - coord = _find_coordinator(hass, device_id) + try: + coord = _find_coordinator(hass, device_id) + except ValueError as err: + raise HomeAssistantError(str(err)) from err await coord.async_set_hours_override(hours)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/power_saver/__init__.py` around lines 89 - 100, Wrap calls to _find_coordinator in both service handlers (handle_set_schedule_hours and handle_clear_override) in a try/except that catches ValueError and raises homeassistant.exceptions.HomeAssistantError with a clear, user-facing message (include the device_id in the message for context); ensure HomeAssistantError is imported at the top of the module if not already. This converts the raw ValueError into a clean Home Assistant service error while preserving the original failure behavior.tests/test_services.py (2)
86-111: These async test functions don't useawait.
test_hours_override_included_in_fingerprintandtest_hours_override_propertyare marked asasyncbut don't contain anyawaitstatements. They should be regular synchronous test functions.♻️ Proposed fix
-async def test_hours_override_included_in_fingerprint(): +def test_hours_override_included_in_fingerprint(): """Test that changing hours_override changes the options fingerprint."""-def test_hours_override_property(): +def test_hours_override_property(): """Test the hours_override property returns current value."""(Note:
test_hours_override_propertyis already sync—onlytest_hours_override_included_in_fingerprintneeds the change)Also applies to: 166-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_services.py` around lines 86 - 111, The test function test_hours_override_included_in_fingerprint is declared async but contains no await calls — make it a regular synchronous test by changing "async def test_hours_override_included_in_fingerprint()" to a normal "def" function; keep the rest of the body (patching DataUpdateCoordinator.__init__, creating PowerSaverCoordinator via __new__, setting coordinator._hours_override and calling coordinator._compute_options_fingerprint()) unchanged. Note that test_hours_override_property is already synchronous and does not need modification.
17-21: This test doesn't validate real behavior.The test creates a
MagicMock, manually sets_hours_override = None, then asserts it'sNone. This doesn't test any actual coordinator logic—it only tests that Python assignment works.Consider either removing this test or testing the actual coordinator initialization (which your other tests already cover better via
__new__pattern).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_services.py` around lines 17 - 21, The test test_hours_override_default_is_none is invalid because it only uses a MagicMock and assigns _hours_override manually; either remove this test or replace it to exercise real coordinator behavior: instantiate a real PowerSaverCoordinator (not a MagicMock) or use the existing __new__-based initialization used elsewhere, then assert the coordinator._hours_override is None to validate default initialization of PowerSaverCoordinator._hours_override.custom_components/power_saver/coordinator.py (1)
547-550: Consider validatinghours_overridetype on restore.The restored value isn't validated. If storage is corrupted with an invalid type (e.g., a string), it could cause runtime issues when used in schedule computation. As per coding guidelines, defensive parsing should be applied.
♻️ Proposed defensive validation
hours_override = data.get("hours_override") - if hours_override is not None: - self._hours_override = hours_override - _LOGGER.info("Restored hours_override: %s", hours_override) + if hours_override is not None: + try: + self._hours_override = float(hours_override) + if not 0 <= self._hours_override <= 24: + _LOGGER.warning( + "Restored hours_override %s out of range, ignoring", + hours_override, + ) + self._hours_override = None + else: + _LOGGER.info("Restored hours_override: %s", self._hours_override) + except (ValueError, TypeError): + _LOGGER.warning( + "Invalid hours_override in storage: %s, ignoring", hours_override + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/power_saver/coordinator.py` around lines 547 - 550, The restored hours_override value is not validated and may be an invalid type from corrupted storage; update the restore logic around the hours_override variable to defensively validate before assigning to self._hours_override: check the type (e.g., isinstance(hours_override, int) or number as appropriate for your domain), ensure it falls within acceptable bounds if applicable, and only then set self._hours_override; otherwise log a warning via _LOGGER (include the bad value) and ignore/reset to a safe default (None or previous value).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/power_saver/__init__.py`:
- Around line 131-135: The code currently removes services even if unload_ok is
False; change the condition to only remove the services when the unload
succeeded and the integration state is gone by updating the check to: if
unload_ok and not hass.data.get(DOMAIN): then call hass.services.async_remove
for SERVICE_SET_SCHEDULE_HOURS and SERVICE_CLEAR_SCHEDULE_HOURS_OVERRIDE (ensure
you reference the existing unload_ok variable and the constants DOMAIN,
SERVICE_SET_SCHEDULE_HOURS, SERVICE_CLEAR_SCHEDULE_HOURS_OVERRIDE).
---
Nitpick comments:
In `@custom_components/power_saver/__init__.py`:
- Around line 89-100: Wrap calls to _find_coordinator in both service handlers
(handle_set_schedule_hours and handle_clear_override) in a try/except that
catches ValueError and raises homeassistant.exceptions.HomeAssistantError with a
clear, user-facing message (include the device_id in the message for context);
ensure HomeAssistantError is imported at the top of the module if not already.
This converts the raw ValueError into a clean Home Assistant service error while
preserving the original failure behavior.
In `@custom_components/power_saver/coordinator.py`:
- Around line 547-550: The restored hours_override value is not validated and
may be an invalid type from corrupted storage; update the restore logic around
the hours_override variable to defensively validate before assigning to
self._hours_override: check the type (e.g., isinstance(hours_override, int) or
number as appropriate for your domain), ensure it falls within acceptable bounds
if applicable, and only then set self._hours_override; otherwise log a warning
via _LOGGER (include the bad value) and ignore/reset to a safe default (None or
previous value).
In `@tests/test_services.py`:
- Around line 86-111: The test function
test_hours_override_included_in_fingerprint is declared async but contains no
await calls — make it a regular synchronous test by changing "async def
test_hours_override_included_in_fingerprint()" to a normal "def" function; keep
the rest of the body (patching DataUpdateCoordinator.__init__, creating
PowerSaverCoordinator via __new__, setting coordinator._hours_override and
calling coordinator._compute_options_fingerprint()) unchanged. Note that
test_hours_override_property is already synchronous and does not need
modification.
- Around line 17-21: The test test_hours_override_default_is_none is invalid
because it only uses a MagicMock and assigns _hours_override manually; either
remove this test or replace it to exercise real coordinator behavior:
instantiate a real PowerSaverCoordinator (not a MagicMock) or use the existing
__new__-based initialization used elsewhere, then assert the
coordinator._hours_override is None to validate default initialization of
PowerSaverCoordinator._hours_override.
🪄 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: 96ccd20d-a074-42d9-a51f-e75a5ee6f637
📒 Files selected for processing (11)
README.mdcustom_components/power_saver/__init__.pycustom_components/power_saver/const.pycustom_components/power_saver/coordinator.pycustom_components/power_saver/sensor.pycustom_components/power_saver/services.yamlcustom_components/power_saver/strings.jsoncustom_components/power_saver/translations/en.jsoncustom_components/power_saver/translations/sv.jsontests/test_sensor.pytests/test_services.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
custom_components/power_saver/__init__.py (1)
94-100: Consider logging warnings before raising HomeAssistantError.The error is caught but not logged before re-raising. As per coding guidelines, defensive error handling should include
_LOGGER.warning()to aid debugging.♻️ Proposed fix
try: coord = _find_coordinator(hass, device_id) except ValueError as err: + _LOGGER.warning( + "Failed to find coordinator for device %s: %s", device_id, err + ) raise HomeAssistantError( f"Device {device_id} not found or not a Power Saver device" ) from errApply similar logging to
handle_clear_override(lines 105-110).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom_components/power_saver/__init__.py` around lines 94 - 100, The try/except around _find_coordinator in the code path that calls coord.async_set_hours_override(hours) should log a warning before re-raising HomeAssistantError: add a _LOGGER.warning call in the except ValueError as err block that includes the device_id and err (or err.args) for context, then raise the HomeAssistantError from err as currently done; apply the same change to the matching error handling in handle_clear_override so both blocks log warnings before raising.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@custom_components/power_saver/__init__.py`:
- Around line 94-100: The try/except around _find_coordinator in the code path
that calls coord.async_set_hours_override(hours) should log a warning before
re-raising HomeAssistantError: add a _LOGGER.warning call in the except
ValueError as err block that includes the device_id and err (or err.args) for
context, then raise the HomeAssistantError from err as currently done; apply the
same change to the matching error handling in handle_clear_override so both
blocks log warnings before raising.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bd0b108-3dec-4a8c-9c2d-32c8a33f8112
📒 Files selected for processing (3)
custom_components/power_saver/__init__.pycustom_components/power_saver/coordinator.pytests/test_services.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_services.py
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/power_saver/coordinator.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #29
Summary by CodeRabbit
New Features
Documentation
Tests