Coordinator: add _async_setup() and collect set of existing devices#982
Coordinator: add _async_setup() and collect set of existing devices#982
Conversation
|
Warning Rate limit exceeded@bouwew has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 23 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)
WalkthroughAdds per-instance device-tracking attributes and a new async setup step to preload stored devices; moves connection establishment into the coordinator update flow using Changes
Sequence Diagram(s)sequenceDiagram
participant C as PlugwiseDataUpdateCoordinator
participant DR as DeviceRegistry
participant API as PlugwiseAPI
participant RO as RegistryOps
Note over C,DR: _async_setup() — preload stored devices
C->>DR: query devices for config_entry (domain=plugwise)
DR-->>C: return stored device IDs (_stored_devices)
Note over C,API: periodic _async_update_data()
C->>API: api.connect()
alt connect success
API-->>C: Version / connected
C->>API: api.async_update()
API-->>C: latest device data (ids)
C->>C: derive current_devices (from _current/_stored), compute new_devices
alt new devices present
C->>RO: create/update entries for new_devices
end
alt devices removed
C->>RO: remove orphaned devices via gateway (if gateway present & via_device_id known)
end
else connect failure
API-->>C: error (InvalidAuthentication/InvalidSetup/Unsupported/Other)
C-->>C: raise ConfigEntryError or UpdateFailed (mapped)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
custom_components/plugwise/coordinator.py (1)
95-99: Confirm/connect error handling now that_connectmoved into_async_setup.
_async_setupcallsawait self._connect()directly. Ifself.api.connect()can raiseConnectionFailedError,InvalidAuthentication, etc., those will now bubble out of_async_setupinstead of being translated toUpdateFailed/ConfigEntryErroras in_async_update_data. That may change how setup failures are surfaced in HA.Consider mirroring the same exception mapping used around
async_update()(or factoring that mapping into a shared helper) so connection failures during initial setup still produce the expected, translated errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
⏰ 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
⏰ 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 (1)
custom_components/plugwise/coordinator.py (1)
100-109: Device registry logic correctly extracts Plugwise identifiers.The set comprehension properly extracts
identifier[1](the string ID) from device entries, filtering byDOMAIN. This ensures_current_devicesis aset[str]that aligns withset(data)in line 149, resolving the type mismatch noted in previous reviews.Based on past review comments, the logic now correctly populates the set with string identifiers rather than
DeviceEntryobjects.
991ff61 to
f739c02
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
custom_components/plugwise/coordinator.py (1)
151-151: Consider adding a comment to clarify the first-run behavior.The ternary expression correctly handles two scenarios:
- First run (
_current_devicesis empty): Compare against_stored_devicesto detect devices that exist in the registry but aren't reported by the API- Subsequent runs: Compare against
_current_devicesto detect devices that disappeared since the last updateWhile the logic is sound, the conditional is dense. A brief inline comment would help future maintainers understand the first-run special case.
- removed_devices = (self._stored_devices - set(data)) if not self._current_devices else (self._current_devices - set(data)) + # On first run use registry devices, otherwise use previous data snapshot + removed_devices = (self._stored_devices - set(data)) if not self._current_devices else (self._current_devices - set(data))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
⏰ 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 (1)
custom_components/plugwise/coordinator.py (1)
77-77: LGTM: Instance variable properly initialized.The
_stored_devicesset is correctly typed and initialized, consistent with the existing device tracking patterns in the coordinator.
3965408 to
bccae2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
custom_components/plugwise/coordinator.py (1)
114-126:_async_setup()is still not invoked anywhere.The method implementation correctly extracts Plugwise device IDs from the registry, but it's never called. Without invoking this before the first data update,
_stored_devicesremains empty, and the removal logic at lines 153-157 cannot detect orphaned devices on startup.Ensure this is called before
async_config_entry_first_refresh()in your integration setup (typically in__init__.py).#!/bin/bash # Verify if _async_setup is called anywhere in the integration rg -n '_async_setup' custom_components/plugwise/ --type=py
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
🪛 Ruff (0.14.7)
custom_components/plugwise/coordinator.py
40-40: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
41-41: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
42-42: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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)
custom_components/plugwise/coordinator.py (3)
101-112: LGTM!The connection logic correctly sets
_connectedbased on the API response and configures the update interval from defaults or custom options.
127-133: LGTM!The lazy connection pattern and data fetch logic are sound. Connection errors properly propagate through
_connect().
153-157: Removal logic depends on_async_setup()being called first.The conditional logic correctly uses
_stored_deviceswhen_current_devicesis empty (first run). However, since_async_setup()is never invoked,_stored_devicesremains empty, soremoved_deviceswill always be an empty set on the first update cycle.This means orphaned devices (removed from the Plugwise system while HA was offline) won't be cleaned up on startup as intended.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
🪛 GitHub Actions: Test PR against HA-core
custom_components/plugwise/coordinator.py
[error] 86-86: W293 Blank line contains whitespace. Remove trailing whitespace on the blank line.
🔇 Additional comments (4)
custom_components/plugwise/coordinator.py (4)
79-81: LGTM! Instance attribute initialization is correct.The mutable class attributes have been properly moved to instance initialization, ensuring each coordinator instance has its own device tracking sets. This resolves the shared-state concern and the RUF012 static analysis warning.
133-152: Data update logic is sound.The method correctly ensures connection before fetching data, with proper error handling that maps specific exceptions to appropriate Home Assistant error types. The debug logging on line 151 provides good visibility into the data fetch operation.
Note: This method depends on
_async_setup()having been called first to properly populate_stored_devicesfor device removal detection.
154-162: Device delta computation is correctly implemented.The logic properly:
- Identifies new devices by comparing against
_current_devices(line 158)- Falls back to
_stored_deviceson the first update when_current_devicesis empty (line 159)- Updates the tracking state (line 160)
- Triggers removal for devices that disappeared (line 161)
This approach ensures proper device lifecycle management across coordinator updates.
173-174: Good defensive programming.The early return when the gateway device is missing prevents potential
Nonereference errors on line 176 and ensures the removal operation only proceeds with a valid gateway reference. This is solid error prevention for edge cases where the gateway might be removed from the registry.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/components/plugwise/test_init.py (1)
121-154: Exception-translation test covers all mapped_connecterrorsThis parametrized test neatly verifies that:
- auth/setup/unsupported exceptions map to
ConfigEntryError, and- connection/XML/response failures map to
UpdateFailed.Using exception classes as
side_effectis valid withMagicMock/AsyncMock, and the membership check for computingexpected_exceptionis fine.If you want to make the test table-driven, you could parametrize the expected exception instead of computing it, but that’s purely stylistic.
custom_components/plugwise/coordinator.py (1)
85-129: Exception translation in_connectlooks correct; consider chaining causesThe mapping from Plugwise exceptions to
ConfigEntryErrorvsUpdateFailedmatches the new tests and gives clear error semantics, andupdate_intervaladjustment (includingCONF_SCAN_INTERVALoverride) preserves existing behavior.Ruff B904 points out that these re-raises currently drop the original exception context. If you want better traceability (and to satisfy the linter), you can capture the original error and chain it:
- except ConnectionFailedError: - raise UpdateFailed( + except ConnectionFailedError as err: + raise UpdateFailed( translation_domain=DOMAIN, translation_key="failed_to_connect", - ) - except InvalidAuthentication: - raise ConfigEntryError( + ) from err + except InvalidAuthentication as err: + raise ConfigEntryError( translation_domain=DOMAIN, translation_key="authentication_failed", - ) - except InvalidSetupError: - raise ConfigEntryError( + ) from err + except InvalidSetupError as err: + raise ConfigEntryError( translation_domain=DOMAIN, translation_key="invalid_setup", - ) - except (InvalidXMLError, ResponseError): - raise UpdateFailed( + ) from err + except (InvalidXMLError, ResponseError) as err: + raise UpdateFailed( translation_domain=DOMAIN, translation_key="invalid_xml_data", - ) - except UnsupportedDeviceError: - raise ConfigEntryError( + ) from err + except UnsupportedDeviceError as err: + raise ConfigEntryError( translation_domain=DOMAIN, translation_key="unsupported_firmware", - ) + ) from errIf you prefer to hide implementation details from logs, you can instead use
from Nonein these raises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/coordinator.py(5 hunks)tests/components/plugwise/test_init.py(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 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_init.py
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-10-08T15:35:25.180Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
Applied to files:
tests/components/plugwise/test_init.py
📚 Learning: 2025-12-03T10:16:54.874Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
Applied to files:
tests/components/plugwise/test_init.pycustom_components/plugwise/coordinator.py
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
🪛 Ruff (0.14.7)
custom_components/plugwise/coordinator.py
93-96: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
98-101: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
103-106: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
108-111: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
113-116: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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
🔇 Additional comments (9)
tests/components/plugwise/test_init.py (3)
6-34: New imports correctly reflect updated test surface
InvalidSetupError,PlugwiseDataUpdateCoordinator,ConfigEntryError, andUpdateFailedare all used in the new tests and correctly scoped. No issues here.
101-119: Gateway not-ready test now focused on transient PlugwiseErrorRestricting
side_effecttoPlugwiseErrorand assertingConfigEntryState.SETUP_RETRYmatches the current flow (PlugwiseError→UpdateFailed→ coordinator retry). The expectations look correct and aligned with the new coordinator behavior.
181-193: RegistryEntry annotation swap is fineChanging the annotation to
er.RegistryEntrymatches the imported alias and is type-equivalent. No behavioral impact.custom_components/plugwise/coordinator.py (6)
5-14: NewInvalidSetupErrorimport is consistent
InvalidSetupErroris now used in_connect’s exception mapping; the import is correct and necessary.
40-43: Per‑instance device sets fix shared-state riskDefining
_current_devices,_stored_devices, andnew_devicesas typed attributes and initializing them in__init__gives each coordinator its own sets and removes the earlier shared mutable class-state concern. This also aligns with the prior Ruff RUF012 hint.Also applies to: 80-82
130-141:_async_setupcorrectly preloads stored device idsUsing the device registry to build
_stored_devicesfrom(DOMAIN, identifier)pairs for this config entry provides the right baseline for detecting removed devices on the first update. This integrates well with the HADataUpdateCoordinatorlifecycle where_async_setupruns before the first refresh. Based on learnings, this matches the intended framework behavior.
143-157: Updated_async_update_dataflow aligns connect + update responsibilitiesEnsuring
_connect()is called whennot self._connectedbeforeapi.async_update(), then delegating diffing to_async_add_remove_devicesand surfacing a singleUpdateFailedpath forPlugwiseErrorkeeps the coordinator responsibilities clear and matches the tests’ expectations for SETUP_RETRY behavior. The extra DEBUG log of the data is fine for troubleshooting.
159-167: Device delta computation is consistent with stored vs current devicesUsing:
set_of_data = set(data)for current ids,self.new_devices = set_of_data - self._current_devices, andcurrent_devices = self._stored_devices if not self._current_devices else self._current_devicesmeans:
- on the first cycle, removals are detected relative to the registry (
_stored_devices), and- on subsequent cycles, removals are detected relative to the previous update (
_current_devices),while always updating
_current_devicesto the latest keys. This matches the goal of cleaning up devices that disappear from the backend while still tracking newly seen ids innew_devices.
176-182: GatewayNone‑guard in_async_remove_devicesis a safe defensive checkGuarding against a missing
gateway_deviceavoids a potentialNonedereference, and cachingvia_device_idkeeps the later loop simpler. The subsequent filtering onvia_device_idandidentifier[1] not in datais unchanged in behavior.
35bf752 to
ffd1d98
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
custom_components/plugwise/coordinator.py (2)
9-11:_connecterror mapping and interval handling are consistent; docstring could be tightened.The new handling for
InvalidSetupErrorand the mapping of XML/response issues toUpdateFailedwith translation keys is aligned with HA’s error model, and deferring interval calculation to after a successful connect viaDEFAULT_SCAN_INTERVALplus optionalCONF_SCAN_INTERVALoverride is sound.Minor nit: the docstring currently says “Returns a Version object when connection succeeds”, but the method does not return that object; consider rephrasing to describe the side effects (setting
_connected/update_interval) instead.Please double-check that the translation keys
"invalid_setup"and"invalid_xml_data"are present in your integration’s translation files so the new errors surface correctly.Also applies to: 86-111, 118-128
162-167: Consider basingnew_devicesoncurrent_devicesso_stored_devicesaffects first-run detection.Right now:
set_of_data = set(data) self.new_devices = set_of_data - self._current_devices current_devices = self._stored_devices if not self._current_devices else self._current_devices self._current_devices = set_of_dataOn the first update,
_current_devicesis empty, sonew_devicesbecomesset_of_dataand_stored_devicesonly influences removal detection. If the intent of_stored_devicesis also to avoid treating already-registered devices as “new” on the first cycle, you could instead computenew_devicesfromcurrent_devices:- set_of_data = set(data) - self.new_devices = set_of_data - self._current_devices - current_devices = self._stored_devices if not self._current_devices else self._current_devices - self._current_devices = set_of_data + set_of_data = set(data) + current_devices = ( + self._stored_devices if not self._current_devices else self._current_devices + ) + self.new_devices = set_of_data - current_devices + self._current_devices = set_of_dataThis preserves existing behavior on subsequent updates while letting
_stored_devicesinfluence new-device detection on the very first run.Please confirm whether
new_devicesis meant to exclude pre-existing registry devices on the first update; if not, the current logic is fine and this change can be skipped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
📚 Learning: 2025-12-03T10:16:54.874Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
Applied to files:
custom_components/plugwise/coordinator.py
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
⏰ 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 (4)
custom_components/plugwise/coordinator.py (4)
41-43: Per-instance device tracking sets look good.Annotating
_current_devices,_stored_devices, andnew_devicesat class scope and initializing them as fresh sets in__init__correctly avoids shared mutable state between coordinator instances and resolves the earlier RUF012 concern.Also applies to: 80-82
130-142: Registry-based_stored_devicesinitialization matches downstream set logic.Building
_stored_devicesas the set of(DOMAIN, id)identifiers fromdevice_entriesensures type compatibility withset(data)in_async_add_remove_devicesand avoids the earlierDeviceEntryvsstrmismatch.Based on learnings, this will be invoked automatically via
DataUpdateCoordinator.async_config_entry_first_refresh(), so no extra manual call is needed.
143-157: Connect-before-update flow in_async_update_datais appropriate.Guarding on
self._connectedand calling_connect()once beforeapi.async_update(), while mapping anyPlugwiseErrortoUpdateFailedwith a clear translation key, fits the coordinator pattern and keeps connection and data-fetch concerns separated. The delegated call to_async_add_remove_devicesafter a successful fetch is also clean.
178-182: GatewayNone-guard in_async_remove_devicesprevents crashes.Early-returning when
gateway_deviceisNoneand then cachingvia_device_id = gateway_device.idbefore the loop avoids a potentialAttributeErrorand keeps the subsequent orphan-removal loop focused on valid gateway-linked devices only.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
custom_components/plugwise/coordinator.py (1)
85-89: Exception translation and interval handling in_connect()look correct; docstring is slightly misleading.Mapping Plugwise connection errors to
ConfigEntryErrorvsUpdateFailedmatches Home Assistant expectations, and dynamically adjustingupdate_interval(with an options override) is coherent with the rest of the integration. Only nit: the docstring says “Returns a Version object when connection succeeds” while_connect()itself returnsNoneand just uses the returnedVersionto set_connectedand the interval—consider rephrasing to describe the side effects rather than a return value.Also applies to: 91-111, 118-128
tests/components/plugwise/test_init.py (1)
121-153:test_coordinator_connect_exceptionsaccurately validates_connect()’s exception mapping.Parametrizing over the Plugwise exception classes and checking for
ConfigEntryErroronly forInvalidAuthentication,InvalidSetupError, andUnsupportedDeviceErrormirrors the_connect()exceptblocks; the other exceptions appropriately map toUpdateFailed. If you ever want to make the expectation logic more explicit, you could replace thein (...)check with a small mapping dict{InvalidAuthentication: ConfigEntryError, ...}but that’s purely stylistic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/coordinator.py(5 hunks)tests/components/plugwise/test_init.py(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
📚 Learning: 2025-12-03T10:16:54.874Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
Applied to files:
custom_components/plugwise/coordinator.pytests/components/plugwise/test_init.py
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 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_init.py
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-10-08T15:35:25.180Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
Applied to files:
tests/components/plugwise/test_init.py
🧬 Code graph analysis (2)
custom_components/plugwise/coordinator.py (1)
custom_components/plugwise/select.py (1)
options(146-148)
tests/components/plugwise/test_init.py (1)
custom_components/plugwise/coordinator.py (1)
_connect(85-128)
⏰ 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/coordinator.py (3)
40-43: Per-instance device tracking state is correctly scoped to the coordinator.Moving
_current_devices,_stored_devices, andnew_devicesinitialization into__init__avoids shared mutable class state and aligns with how_async_add_remove_devices()uses these sets over the coordinator lifecycle. No further changes needed here.Also applies to: 80-82
130-142: Preloading_stored_devicesfrom the device registry and the updated refresh flow look consistent.
_async_setup()now builds_stored_devicesas a set of Plugwise identifiers fromDeviceEntry.identifiers, which matches the key type used indataand allows first-run removal detection to work._async_update_data()correctly ensures a connection via_connect()before callingasync_update()and normalizes anyPlugwiseErrorinto a singleUpdateFailed("data_incomplete_or_missing"), which simplifies caller handling. This all fits the DataUpdateCoordinator pattern; no issues spotted.Also applies to: 143-153
159-169: Device delta computation and guarded removal logic look robust.Using
set(data)for keys,self.new_devices = set_of_data - self._current_devices, and falling back to_stored_deviceson the first run gives the expected behavior: all currently reported devices are “new” at startup, while removals are derived from previously stored identifiers. The_async_remove_devices()guard for a missing gateway device avoids a potentialNone-dereference, and filtering byvia_device_idensures only child devices are removed while the gateway itself is preserved. The implementation matches the intent described in the changelog and the associated tests.Also applies to: 171-184
tests/components/plugwise/test_init.py (2)
6-14: Imports and type annotation changes line up with the new coordinator behavior.Bringing in
InvalidSetupError,PlugwiseDataUpdateCoordinator,ConfigEntryError, andUpdateFailedgives the tests direct access to the coordinator and its translated exceptions, and switching the migration helper annotation toer.RegistryEntrymatches the existingentity_registry as erimport. All of this is consistent and keeps the tests tightly coupled to the current APIs.Also applies to: 19-20, 31-34, 191-191
101-104: Gateway “not ready” test now cleanly asserts the retry state.Constraining
side_effecttoPlugwiseErrorand assertingConfigEntryState.SETUP_RETRYdirectly matches the new_async_update_data()behavior (wrappingPlugwiseErrorinUpdateFailedand causing Home Assistant to retry setup). The test remains focused and readable.Also applies to: 111-118
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/plugwise/coordinator.py(5 hunks)tests/components/plugwise/test_init.py(7 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-06-22T08:19:52.059Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
📚 Learning: 2025-08-23T16:26:34.700Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 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_init.py
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-10-08T15:35:25.180Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
Applied to files:
tests/components/plugwise/test_init.py
📚 Learning: 2025-12-03T10:16:54.874Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 982
File: custom_components/plugwise/coordinator.py:120-132
Timestamp: 2025-12-03T10:16:54.874Z
Learning: In Home Assistant integrations, when a coordinator extends DataUpdateCoordinator and defines an `_async_setup()` method, it is automatically called by the framework during `async_config_entry_first_refresh()` before the first data update. The framework calls `__wrap_async_setup()` which invokes `_async_setup()`, so there's no need to manually call it during coordinator initialization.
Applied to files:
tests/components/plugwise/test_init.pycustom_components/plugwise/coordinator.py
📚 Learning: 2024-10-08T15:35:25.180Z
Learnt from: bouwew
Repo: plugwise/plugwise-beta PR: 671
File: custom_components/plugwise/binary_sensor.py:0-0
Timestamp: 2024-10-08T15:35:25.180Z
Learning: The suggestion to initialize `devices` directly in the setup function was reverted by user bouwew. Future suggestions should consider the project's specific requirements and preferences more closely.
Applied to files:
custom_components/plugwise/coordinator.py
🧬 Code graph analysis (1)
tests/components/plugwise/test_init.py (2)
custom_components/plugwise/coordinator.py (1)
_connect(85-128)tests/components/plugwise/conftest.py (2)
mock_smile_anna(240-263)mock_smile_adam_heat_cool(185-208)
🪛 GitHub Actions: Test PR against HA-core
tests/components/plugwise/test_init.py
[error] 406-406: Ruff: F841 Local variable 'data' is assigned to but never used.
🪛 GitHub Check: SonarCloud Code Analysis
tests/components/plugwise/test_init.py
[warning] 406-406: Remove the unused local variable "data".
🪛 Ruff (0.14.7)
tests/components/plugwise/test_init.py
406-406: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
🔇 Additional comments (11)
tests/components/plugwise/test_init.py (6)
6-14: LGTM!The imports correctly align with the new coordinator exception handling being tested.
InvalidSetupErroris now mapped toConfigEntryErrorin the coordinator.
17-33: LGTM!New imports for
PlugwiseDataUpdateCoordinator,ConfigEntryError, andUpdateFailedare properly added to support the new exception translation tests.
103-118: LGTM!Test correctly verifies that a
PlugwiseErrorduringasync_updateresults inConfigEntryState.SETUP_RETRY. The simplified parametrization is cleaner.
121-149: LGTM!Comprehensive parameterized test that verifies all exception mappings in
_connect():
ConnectionFailedError→UpdateFailedInvalidAuthentication,InvalidSetupError,UnsupportedDeviceError→ConfigEntryErrorInvalidXMLError,ResponseError→UpdateFailedThis aligns with the coordinator implementation in lines 92-116.
187-187: LGTM!Type annotation correctly uses the
eralias that's already imported, improving consistency with other usages in the file.
387-413: Test logic looks correct for verifying device cleanup.The test correctly:
- Pre-creates an orphan device before setup
- Verifies it exists in the registry
- Runs component setup
- Verifies the orphan device is removed
This exercises the
_async_remove_devicespath in the coordinator.custom_components/plugwise/coordinator.py (5)
41-43: LGTM!Instance attributes are now properly declared as type annotations at class level and initialized in
__init__, resolving the previous RUF012 concern about mutable class-level defaults.Also applies to: 80-82
85-128: LGTM!The
_connectmethod correctly:
- Maps exceptions to appropriate HA error types (
ConfigEntryErrorfor auth/setup/device issues,UpdateFailedfor transient connection/parsing issues)- Sets
_connectedbased on receiving a validVersionobject- Configures
update_intervalfrom defaults or user options
130-141: LGTM!The
_async_setupmethod correctly extracts Plugwise device identifiers from the device registry, populating_stored_deviceswithset[str]values that are comparable toset(data). Based on learnings, this is automatically invoked by the HA framework duringasync_config_entry_first_refresh().
159-169: LGTM!The device delta logic is well-structured:
new_devicescorrectly identifies devices in data but not in_current_devices- Falls back to
_stored_deviceson first run when_current_devicesis empty- Triggers removal only when there are actual devices to remove
This properly handles both initial setup and subsequent updates.
180-183: Good defensive guard against missing gateway device.The early return prevents potential
AttributeErrorif the gateway device is unexpectedly absent from the registry.
fb05702 to
cf5c3a1
Compare
ebc5d3b to
f88e383
Compare
f88e383 to
d87cdc2
Compare
c53f211 to
6367335
Compare
|
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
Chore
✏️ Tip: You can customize this high-level summary in your review settings.