Merge ASYNC-branch into MAIN #238
Conversation
This migrates the unique ID from previous threaded version to the ones used at this async version
WalkthroughThis update delivers a major refactor and modernization of the Plugwise USB Home Assistant integration. It introduces new coordinators, entity structures, and dynamic event-driven entity management. Manual device add/remove services are replaced by an auto-joining service. The release also upgrades dependencies, testing infrastructure, and translation/localization, and introduces a development container. Changes
Sequence Diagram(s)sequenceDiagram
participant HA as Home Assistant
participant Plugwise as Plugwise USB Integration
participant Stick as Plugwise USB Stick
participant Node as Plugwise Node
HA->>Plugwise: async_setup_entry(config_entry)
Plugwise->>Stick: Initialize Stick (async)
Stick-->>Plugwise: Stick ready
Plugwise->>HA: Register Stick device
Plugwise->>Stick: Subscribe to NodeEvent.DISCOVERED
Stick-->>Plugwise: Node discovered
Plugwise->>Node: Create DataUpdateCoordinator
Plugwise->>HA: Add node entities (sensor, switch, etc.)
HA->>Plugwise: Service call: enable_auto_joining
Plugwise->>Stick: Enable auto-joining
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (4)
custom_components/plugwise_usb/binary_sensor.py (1)
102-124: 🛠️ Refactor suggestionRemove redundant None checks in coordinator update
The method has duplicate None checks for the coordinator data. The second check (lines 113-118) can never be reached if the first check (lines 106-112) passes.
@callback def _handle_coordinator_update(self) -> None: """Handle updated data from the coordinator.""" data = self.coordinator.data.get(self.entity_description.node_feature, None) if data is None: _LOGGER.debug( "No %s binary sensor data for %s", self.entity_description.node_feature, self._node_info.mac, ) return - if self.coordinator.data[self.entity_description.node_feature] is None: - _LOGGER.info( - "No binary sensor data for %s", - str(self.entity_description.node_feature), - ) - return self._attr_is_on = getattr( self.coordinator.data[self.entity_description.node_feature], self.entity_description.api_attribute, ) self.async_write_ha_state()custom_components/plugwise_usb/switch.py (1)
144-153:⚠️ Potential issueAdd error handling to async switch methods.
The
async_turn_onandasync_turn_offmethods don't handle potential failures from the async switch function calls. This could lead to inconsistent state if the operation fails.Apply this fix to add proper error handling:
async def async_turn_on(self, **kwargs): """Turn the switch on.""" - self._attr_is_on = await self.async_switch_fn(True) - self.async_write_ha_state() + try: + result = await self.async_switch_fn(True) + self._attr_is_on = result + self.async_write_ha_state() + except Exception as err: + _LOGGER.error("Failed to turn on switch: %s", err) + # Refresh state from coordinator to ensure consistency + await self.coordinator.async_request_refresh() async def async_turn_off(self, **kwargs): """Turn the switch off.""" - self._attr_is_on = await self.async_switch_fn(False) - self.async_write_ha_state() + try: + result = await self.async_switch_fn(False) + self._attr_is_on = result + self.async_write_ha_state() + except Exception as err: + _LOGGER.error("Failed to turn off switch: %s", err) + # Refresh state from coordinator to ensure consistency + await self.coordinator.async_request_refresh()custom_components/plugwise_usb/sensor.py (1)
221-242:⚠️ Potential issueFix potential KeyError in coordinator update handler.
Similar to the switch component, this method could raise a KeyError when accessing the coordinator data.
Apply this fix:
@callback def _handle_coordinator_update(self) -> None: """Handle updated data from the coordinator.""" data = self.coordinator.data.get(self.entity_description.node_feature, None) if data is None: _LOGGER.debug( "No %s sensor data for %s", self.entity_description.node_feature, self._node_info.mac, ) return - self._attr_native_value = getattr( - self.coordinator.data[self.entity_description.node_feature], - self.entity_description.key, - ) + self._attr_native_value = getattr( + data, + self.entity_description.key, + ) if self.entity_description.node_feature == NodeFeature.ENERGY: self._attr_last_reset = getattr( - self.coordinator.data[self.entity_description.node_feature], + data, f"{self.entity_description.key}_reset", ) self.async_write_ha_state()custom_components/plugwise_usb/config_flow.py (1)
31-54:⚠️ Potential issueFix incorrect function signature.
The
validate_usb_connectionfunction hasselfas its first parameter but it's not a class method. This causes a mismatch with how it's called on lines 87 and 113 whereself.hassis passed as the first argument.Apply this fix to correct the function signature:
-async def validate_usb_connection(self, device_path=None) -> tuple[dict[str, str], str]: +async def validate_usb_connection(hass, device_path=None) -> tuple[dict[str, str], str]: """Test if device_path is a real Plugwise USB-Stick.""" errors = {} # Avoid creating a 2nd connection to an already configured stick - if device_path in plugwise_stick_entries(self): + if device_path in plugwise_stick_entries(hass): errors[CONF_BASE] = "already_configured" return errors, None
🧹 Nitpick comments (13)
scripts/python-venv.sh (1)
6-6: Adopt hidden virtual environment directory
Switching fromvenvto.venvfollows common conventions for a hidden project venv. Ensure that.gitignorehas been updated accordingly.requirements_commit.txt (1)
3-3: Addruffto commit requirements
Includingruffhere aligns with the new linting steps in CI. Consider pinning its version to avoid unexpected upgrades.requirements_test.txt (1)
1-7: Add testing dependencies
The newrequirements_test.txtenumerates all testing frameworks, includingpytest,pytest-asyncio, and the direct URL forplugwise-usb-0.40.0.Consider pinning the URL with a hash (PEP 610) to ensure package integrity, for example:
- plugwise-usb @ https://files.pythonhosted.org/.../plugwise_usb-0.40.0.tar.gz + plugwise-usb @ https://files.pythonhosted.org/.../plugwise_usb-0.40.0.tar.gz \ + --hash=sha256:<checksum>CHANGELOG.md (1)
5-17: Clarify phrasing in changelog entry
The bullet- Replace the device-add service by the enable auto-joining servicereads a bit awkwardly. Consider rephrasing to:
- Replace the device-add service with the auto-joining serviceor
- Enable auto-joining service instead of manual device-addfor improved readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~8-~8: The verb ‘enable’ does not usually follow articles like ‘the’. Check that ‘enable’ is spelled correctly; using ‘enable’ as a noun may be non-standard.
Context: ....0) - Replace the device-add service by the enable auto-joining service - Remove device-re...(A_INFINITIVE)
.github/workflows/test.yml (1)
23-38: CI performance improvement: Cache Python dependencies
Currently each run rebuilds the virtual environment from scratch. Consider adding anactions/cachestep to persist pip’s cache or the.venvdirectory, which can substantially reduce setup time. For example:- name: Cache pip uses: actions/cache@v3 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements_test.txt') }} restore-keys: | ${{ runner.os }}-pip-scripts/tests_and_coverage.sh (1)
34-38: Address the TODO comment about skipped ruff checks.The commented-out ruff checks suggest there are current code quality issues that need to be addressed.
Would you like me to help create a plan to systematically address the ruff check issues, or should this TODO be tracked in a separate issue?
.devcontainer/devcontainer.json (1)
23-30: Consider adding the Ruff extension.Since the configuration references ruff for formatting, consider adding the official Ruff VS Code extension for better integration.
"extensions": [ "ms-python.python", "ms-python.pylint", + "charliermarsh.ruff", "github.vscode-pull-request-github", "ryanluker.vscode-coverage-gutters", "ms-python.vscode-pylance", "donjayamanne.python-extension-pack" ],tests/conftest.py (2)
57-69: Remove commented-out code or complete the implementation.The commented-out
mock_comportfixture appears to be incomplete and unused. Either complete the implementation if needed for testing serial port enumeration, or remove it to keep the code clean.-# @pytest.fixture -# def mock_comport() -> Generator[MagicMock]: -# """Return a mocked comport.""" -# with patch( -# "serial.tools.list_ports.comports", -# ) as port: -# port = serial.tools.list_ports_common.ListPortInfo(TEST_USBPORT) -# port.serial_number = "1234" -# port.manufacturer = "Virtual serial port" -# port.device = TEST_USBPORT -# port.description = "Some serial port" -# yield [port]
116-121: Fix function documentation comment.The docstring incorrectly mentions "ezbeq integration" instead of "plugwise_usb integration".
async def setup_integration(hass: HomeAssistant, config_entry: MockConfigEntry) -> None: - """Set up the ezbeq integration.""" + """Set up the plugwise_usb integration.""" config_entry.add_to_hass(hass)tests/test_config_flow.py (2)
79-81: Remove unusedinit_integrationparameterThe
init_integrationparameter is not used within the test function. If it's not needed, consider removing it to keep the test clean.-async def test_user_flow_manual( - hass, mock_usb_stick: MagicMock, init_integration: MockConfigEntry -): +async def test_user_flow_manual(hass, mock_usb_stick: MagicMock):
102-121: Consider using a mock fixture for consistencyThis test doesn't use any USB stick mock fixture, which might make it dependent on the actual serial port behavior. Consider using
mock_usb_stick_errorfixture for more predictable test behavior.-async def test_invalid_connection(hass): +async def test_invalid_connection(hass, mock_usb_stick_error: MagicMock):custom_components/plugwise_usb/entity.py (1)
84-99: Unnecessary None check fornode_ducThe
self.node_ducis assigned in__init__and should never beNone. This defensive check might hide actual issues if the coordinator is unexpectedly None.async def async_push_event(self, feature: NodeFeature, state: Any) -> None: """Update data on pushed event.""" - if self.node_duc is None: - _LOGGER.warning( - "Unable to push event=%s, state=%s, mac=%s", - feature, - state, - self._node_info.mac, - ) - else: - self.node_duc.async_set_updated_data( - { - feature: state, - } - ) + self.node_duc.async_set_updated_data( + { + feature: state, + } + )custom_components/plugwise_usb/__init__.py (1)
136-140: Consider adding a timeout for network discovery.The current implementation could wait indefinitely if network discovery fails. Consider adding a timeout to prevent potential deadlock.
+ timeout = 60 # seconds + waited = 0 while True: await asyncio.sleep(1) + waited += 1 if api_stick.network_discovered: break + if waited >= timeout: + _LOGGER.warning("Network discovery timed out after %d seconds", timeout) + break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.devcontainer/devcontainer.json(1 hunks).gitattributes(1 hunks).github/workflows/test.yml(2 hunks).gitignore(1 hunks)CHANGELOG.md(1 hunks)custom_components/plugwise_usb/__init__.py(1 hunks)custom_components/plugwise_usb/binary_sensor.py(1 hunks)custom_components/plugwise_usb/config_flow.py(5 hunks)custom_components/plugwise_usb/const.py(2 hunks)custom_components/plugwise_usb/coordinator.py(1 hunks)custom_components/plugwise_usb/entity.py(1 hunks)custom_components/plugwise_usb/manifest.json(1 hunks)custom_components/plugwise_usb/models.py(0 hunks)custom_components/plugwise_usb/number.py(1 hunks)custom_components/plugwise_usb/sensor.py(1 hunks)custom_components/plugwise_usb/services.yaml(1 hunks)custom_components/plugwise_usb/strings.json(2 hunks)custom_components/plugwise_usb/switch.py(1 hunks)custom_components/plugwise_usb/translations/en.json(2 hunks)custom_components/plugwise_usb/translations/nl.json(2 hunks)hacs.json(1 hunks)pyproject.toml(4 hunks)requirements_commit.txt(1 hunks)requirements_test.txt(1 hunks)scripts/core-testing.sh(1 hunks)scripts/python-venv.sh(1 hunks)scripts/setup_test.sh(1 hunks)scripts/tests_and_coverage.sh(1 hunks)tests/__init__.py(1 hunks)tests/bandit.yaml(0 hunks)tests/components/plugwise_usb/__init__.py(0 hunks)tests/components/plugwise_usb/conftest.py(0 hunks)tests/conftest.py(1 hunks)tests/ruff.toml(1 hunks)tests/test_config_flow.py(11 hunks)
💤 Files with no reviewable changes (4)
- tests/bandit.yaml
- tests/components/plugwise_usb/init.py
- tests/components/plugwise_usb/conftest.py
- custom_components/plugwise_usb/models.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
custom_components/plugwise_usb/entity.py (1)
custom_components/plugwise_usb/coordinator.py (1)
PlugwiseUSBDataUpdateCoordinator(25-87)
tests/test_config_flow.py (1)
tests/conftest.py (4)
mock_usb_stick(72-83)init_integration(45-54)mock_usb_stick_error(87-98)mock_usb_stick_init_error(102-113)
custom_components/plugwise_usb/__init__.py (5)
custom_components/plugwise_usb/coordinator.py (1)
PlugwiseUSBDataUpdateCoordinator(25-87)custom_components/plugwise_usb/sensor.py (2)
async_setup_entry(165-207)async_unload_entry(210-215)custom_components/plugwise_usb/number.py (2)
async_setup_entry(102-140)async_unload_entry(143-148)custom_components/plugwise_usb/binary_sensor.py (2)
async_setup_entry(48-88)async_unload_entry(91-96)custom_components/plugwise_usb/switch.py (2)
async_setup_entry(66-104)async_unload_entry(107-112)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~8-~8: The verb ‘enable’ does not usually follow articles like ‘the’. Check that ‘enable’ is spelled correctly; using ‘enable’ as a noun may be non-standard.
Context: ....0) - Replace the device-add service by the enable auto-joining service - Remove device-re...
(A_INFINITIVE)
🔇 Additional comments (49)
scripts/core-testing.sh (1)
8-8: Approve updated comment for branch override
The change correctly updates the environment variable reference from the outdatedHABRANCHtoBRANCH, aligning the script comments with the logic in.github/workflows/test.yml.hacs.json (1)
3-3: Approve Home Assistant version bump
The minimum required HA version has been correctly updated to2024.8.0, matching the integration’s new manifest and dependency requirements.scripts/python-venv.sh (1)
14-15: Seed venv withuvand installuvpackage
Usinguv venv --seedand then installinguvinside the environment bootstraps the latest tools. This aligns with the new devcontainer setup and guarantees consistent tooling.tests/__init__.py (2)
3-3: Importpytestfor asyncio support
Correctly importspytestto enable usage ofpytestmark.
5-5: Globally applypytest.mark.asyncio
Applyingpytestmark = pytest.mark.asyncioat the module level is a good practice for enabling async tests without decorating each test individually.requirements_commit.txt (1)
1-1: Include test requirements file
Adding-r requirements_test.txtensures that test dependencies are correctly pulled in. Verify thatrequirements_test.txtexists and lists all necessary pytest plugins..gitattributes (1)
1-2: Enforce consistent LF line endings
The.gitattributesfile correctly sets* text=auto eol=lfand explicitly marks Python files as text, aligning with cross-platform consistency goals.Also applies to: 4-5, 7-8
custom_components/plugwise_usb/manifest.json (3)
5-5: Verify removal of former code owner
The code owner list was updated to["@CoMPaTech", "@bouwew", "@brefra"]. Please confirm that removing@dirixmjmis intentional and that review coverage remains adequate.
11-11: Pin runtime dependency for async API
Lockingplugwise-usb==0.40.0ensures compatibility with the new async implementation. This matches the updated library version.
12-12: Ensure version bump consistency
The integration version was incremented to0.50.0a38. Verify this aligns with the pyproject.toml version and release artifacts.pyproject.toml (2)
38-38: Formatting consistency
Adding a blank line improves readability between sections.
219-226: Configure pytest for async and coverage
The[tool.pytest.ini_options]block correctly sets up coverage, asyncio, test paths, and CLI logging for tests.tests/ruff.toml (1)
2-12: Extend and customize Ruff rules for tests
Leveragingextend = "../pyproject.toml"and ignoring B904, N815, RUF018, and SLF001 tailors linting for our test suite..gitignore (2)
5-8: Additional build and virtual environment artifacts ignored appropriately
These new entries (.venv, build, dist, *.egg-info) align with the introduced development container and packaging workflows.
14-14: Ignore coverage output
Adding.coverageensures test coverage files don’t clutter version control.scripts/tests_and_coverage.sh (1)
26-26: Consider the implications of automatic test re-runs.The fallback pytest command with debug logging could mask real test failures by making them appear to pass after a retry. Consider whether this behavior is intentional.
Is the automatic re-run with debug logging intended to handle flaky tests, or should test failures be immediately visible without retry logic?
custom_components/plugwise_usb/const.py (3)
14-19: LGTM! New constants support the async refactor well.The new constants
UNSUB_NODE_LOADED,COORDINATOR,MANUAL_PATH, andNODESproperly support the event-driven async architecture. The naming is clear and follows established conventions.
24-29: Good addition of Platform.NUMBER support.The platform list update to include
Platform.NUMBERand the rename fromPLATFORMS_USBtoPLUGWISE_USB_PLATFORMSimproves clarity and extends functionality.
34-34: Service constant change aligns with async architecture.The change from
SERVICE_USB_DEVICE_ADDtoSERVICE_AUTO_JOINwith the value"enable_auto_joining"reflects the move away from manual device management to automatic discovery, which is appropriate for the async refactor.custom_components/plugwise_usb/translations/en.json (3)
26-29: Clear and helpful auto-join service description.The new auto-join service translation clearly explains the two-step process (enable service, then reset device) which will help users understand the workflow.
83-161: Comprehensive entity translations enhance user experience.The extensive entity translation section provides clear, descriptive names for all entity types (binary sensors, sensors, switches, numbers). This greatly improves the user experience by making entity purposes immediately clear in the UI.
78-78: Minor capitalization improvement.Good fix on the capitalization in the clock-sync interval description for consistency.
custom_components/plugwise_usb/coordinator.py (2)
22-22: Modern type alias implementation looks good.The use of generic type alias for
PlugwiseUSBConfigEntryis a clean approach that provides better type safety and intellisense support.
81-85: Good availability check for non-battery devices.The logic to check device availability for non-battery-powered nodes is appropriate, as these devices should remain connected and responsive unlike battery-powered devices that may sleep.
custom_components/plugwise_usb/translations/nl.json (2)
26-29: Service translation aligns with integration changes.The replacement of manual device add/remove services with the auto-joining service is correctly reflected in the Dutch translation, maintaining consistency with the codebase changes.
83-161: Comprehensive entity translations added.The extensive entity section provides proper Dutch localization for all entity types (binary_sensor, sensor, switch, number), which enhances the user experience for Dutch-speaking users.
custom_components/plugwise_usb/strings.json (3)
26-29: Service change properly documented.The replacement of manual device management with auto-joining is clearly described, providing users with proper guidance on the new workflow.
91-91: Good use of component key references.Using
[%key:component::sensor::entity_component::power::name%]for the power sensor follows Home Assistant best practices for consistent naming across integrations.
121-124: Consistent use of standard component references.The temperature and humidity sensors properly reference standard Home Assistant component keys, ensuring consistency with other integrations.
tests/test_config_flow.py (5)
3-8: Import changes look good!The imports are properly organized and include the necessary modules for testing the config flow with the new async architecture.
30-63: Well-structured test for USB port selection flow!The test properly handles async operations with
await hass.async_block_till_done()and includes good coverage for both successful configuration and duplicate prevention. The consistent use of.get()method for assertions improves readability.
135-146: Good exception handling test pattern!The test properly validates that empty connections are rejected with
InvalidDataexception. The try/except pattern withpytest.failis appropriate here.
148-166: Excellent error handling test!The test properly uses the
mock_usb_stick_errorfixture to simulate connection failures and verifies the correct error response.
168-186: Well-implemented initialization failure test!The test correctly uses the
mock_usb_stick_init_errorfixture to simulate initialization failures and verifies the appropriate error response with the "stick_init" error key.custom_components/plugwise_usb/entity.py (3)
21-26: Clean dataclass definition!The
PlugwiseUSBEntityDescriptiondataclass properly extendsEntityDescriptionwith thenode_featureattribute, providing a clear structure for Plugwise entity descriptions.
42-43: Simplify type annotationConsider using
Noneas the default type instead ofCallable[[], None] | None.- self.unsubscribe_push_events: Callable[[], None] | None = None + self.unsubscribe_push_events: Callable[[], None] | None = NoneActually, this is already optimal. The current type annotation is correct and clear.
53-67: Comprehensive device information!The
device_infoproperty provides all necessary device details including hardware/software versions, Zigbee connection, and proper device hierarchy throughvia_device.custom_components/plugwise_usb/binary_sensor.py (2)
28-45: Well-structured binary sensor entity description!The dataclass properly extends both Plugwise and Home Assistant entity descriptions. The
BINARY_SENSOR_TYPEStuple provides a clean way to define supported binary sensors, currently supporting motion detection.
48-89: Excellent event-driven entity setup!The implementation properly subscribes to node events and dynamically creates entities based on node features. The handling of already loaded nodes ensures no entities are missed during startup.
custom_components/plugwise_usb/switch.py (3)
1-26: LGTM!The imports and constants are well-organized, following the async/coordinator pattern used throughout the integration.
28-64: Well-structured entity descriptions!The dataclass design and switch type definitions follow Home Assistant best practices with clear separation of concerns.
66-113: Excellent event-driven implementation!The setup correctly subscribes to node events and handles both new and existing nodes. The unload function properly cleans up subscriptions.
custom_components/plugwise_usb/sensor.py (3)
2-44: LGTM!The imports and dataclass structure are consistent with the integration's architecture.
45-163: Excellent sensor configuration!The sensor types are comprehensive and properly configured with appropriate device classes, state classes, units, and display precision. Good use of entity categories for diagnostic sensors.
165-216: Well-structured setup with proper event handling!The async setup and unload functions follow the same robust pattern as the switch component, with good debug logging.
custom_components/plugwise_usb/config_flow.py (2)
1-30: LGTM!The imports and helper function are clean and well-organized.
56-126: Well-implemented config flow with proper validation!The config flow correctly handles both automatic USB port detection and manual path entry. Good use of MAC addresses for unique IDs to prevent duplicate configurations.
custom_components/plugwise_usb/__init__.py (2)
206-230: Entity migration logic correctly reverts previous migrations.The migration logic has been inverted to revert previous unique ID changes, which aligns with the PR's objective to deliver a major refactor of the integration.
50-64: Well-structured async initialization with proper error handling.The stick initialization now properly uses async/await pattern with comprehensive error handling and informative error messages.
| *.py text | ||
| *.sh lf |
There was a problem hiding this comment.
Fix invalid .gitattributes attribute for shell scripts
The line *.sh lf uses an unsupported attribute lf. To enforce LF line endings for .sh files, combine text with eol=lf.
Proposed fix:
-*.sh lf
+*.sh text eol=lf📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| *.py text | |
| *.sh lf | |
| *.py text | |
| -*.sh lf | |
| +*.sh text eol=lf |
🤖 Prompt for AI Agents
In the .gitattributes file at lines 8 to 9, the attribute `lf` used for `*.sh`
files is invalid. Replace `*.sh lf` with `*.sh text eol=lf` to correctly enforce
LF line endings for shell scripts.
pyproject.toml
Outdated
| [project] | ||
| name = "plugwise_usb-beta" | ||
| version = "0.40.7" | ||
| version = "0.50.0a13" |
There was a problem hiding this comment.
Critical: Sync project version with manifest
The version is set to 0.50.0a13 here but manifest.json specifies 0.50.0a38. These must match to avoid release inconsistencies.
Proposed fix:
-version = "0.50.0a13"
+version = "0.50.0a38"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| version = "0.50.0a13" | |
| -version = "0.50.0a13" | |
| +version = "0.50.0a38" |
🤖 Prompt for AI Agents
In pyproject.toml at line 3, the version is set to 0.50.0a13, which does not
match the version 0.50.0a38 specified in manifest.json. Update the version in
pyproject.toml to 0.50.0a38 to ensure both files have consistent versioning and
avoid release inconsistencies.
| enable_auto_joining: | ||
| description: First enable auto-joining, then reset the Plugwise devices |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New auto-joining service defined correctly
The enable_auto_joining service replaces manual add/remove operations with a streamlined, single-step approach. Ensure that any references to the old device_add/device_remove services have been removed throughout the codebase.
Run the following to confirm no residual service definitions remain:
🏁 Script executed:
#!/bin/bash
# Verify old services are removed
rg -n "device_add|device_remove" -C2 custom_components/plugwise_usbLength of output: 480
Residual old service definition detected
The SERVICE_USB_DEVICE_REMOVE constant in custom_components/plugwise_usb/const.py (line 35) still defines the old device_remove service. Please remove this (and any leftover device_add) constant to complete the transition to enable_auto_joining.
• custom_components/plugwise_usb/const.py, line 35: delete
- SERVICE_USB_DEVICE_REMOVE: Final = "device_remove"🤖 Prompt for AI Agents
In custom_components/plugwise_usb/const.py at line 35, remove the
SERVICE_USB_DEVICE_REMOVE constant that defines the old device_remove service,
as well as any similar leftover constants like device_add, to fully transition
to the new enable_auto_joining service and eliminate residual old service
definitions.
| echo "Virtualenv available, bailing out" | ||
| exit 2 |
There was a problem hiding this comment.
Fix misleading error message
On failure, the script reports "Virtualenv available, bailing out", but it should indicate absence of the virtualenv. Otherwise users may be confused.
Apply this diff:
- echo "Virtualenv available, bailing out"
+ echo "Virtualenv not available, bailing out"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Virtualenv available, bailing out" | |
| exit 2 | |
| echo "Virtualenv not available, bailing out" | |
| exit 2 |
🤖 Prompt for AI Agents
In scripts/setup_test.sh at lines 18 to 19, the message "Virtualenv available,
bailing out" is misleading on failure. Change the echo statement to indicate
that the virtualenv is not available or missing, so users understand the actual
problem before the script exits with code 2.
There was a problem hiding this comment.
The text to echo is correct, the script should bail out when virtualenv is available.
There was a problem hiding this comment.
@bouwew, I understand you're clarifying the intended behavior. I notice there's a discrepancy between your stated intention and the current implementation:
- You mentioned "the script should bail out when virtualenv is available"
- However, the current code bails out when the virtualenv is not available (when
${my_venv}/bin/activatedoesn't exist)
The current logic activates and uses the virtualenv when it's available, and only bails out when it's missing. Could you clarify if the current implementation matches your intended behavior, or if there should be a change to the conditional logic?
✏️ Learnings added
Learnt from: bouwew
PR: plugwise/plugwise_usb-beta#238
File: scripts/setup_test.sh:18-19
Timestamp: 2025-05-25T17:30:59.154Z
Learning: For scripts/setup_test.sh, the error message "Virtualenv available, bailing out" is considered correct by bouwew, and the script should bail out when virtualenv is available according to the maintainer's preference.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| async def async_unload_entry( | ||
| hass: HomeAssistant, | ||
| config_entry: PlugwiseUSBConfigEntry, | ||
| ) -> None: | ||
| """Unload a config entry.""" | ||
| config_entry.runtime_data[Platform.NUMBER][UNSUB_NODE_LOADED]() | ||
|
|
There was a problem hiding this comment.
Add error handling for potentially missing runtime data.
The unload function should handle cases where the platform setup may not have completed successfully.
async def async_unload_entry(
hass: HomeAssistant,
config_entry: PlugwiseUSBConfigEntry,
) -> None:
"""Unload a config entry."""
- config_entry.runtime_data[Platform.NUMBER][UNSUB_NODE_LOADED]()
+ if Platform.NUMBER in config_entry.runtime_data:
+ if UNSUB_NODE_LOADED in config_entry.runtime_data[Platform.NUMBER]:
+ config_entry.runtime_data[Platform.NUMBER][UNSUB_NODE_LOADED]()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def async_unload_entry( | |
| hass: HomeAssistant, | |
| config_entry: PlugwiseUSBConfigEntry, | |
| ) -> None: | |
| """Unload a config entry.""" | |
| config_entry.runtime_data[Platform.NUMBER][UNSUB_NODE_LOADED]() | |
| async def async_unload_entry( | |
| hass: HomeAssistant, | |
| config_entry: PlugwiseUSBConfigEntry, | |
| ) -> None: | |
| """Unload a config entry.""" | |
| if Platform.NUMBER in config_entry.runtime_data: | |
| if UNSUB_NODE_LOADED in config_entry.runtime_data[Platform.NUMBER]: | |
| config_entry.runtime_data[Platform.NUMBER][UNSUB_NODE_LOADED]() |
🤖 Prompt for AI Agents
In custom_components/plugwise_usb/number.py around lines 143 to 149, the
async_unload_entry function calls a method on runtime_data without checking if
the data or the key exists, which can cause errors if the platform setup was
incomplete. Add error handling to verify that config_entry.runtime_data contains
the Platform.NUMBER key and that UNSUB_NODE_LOADED is callable before invoking
it, and safely handle the case where these are missing to prevent exceptions
during unload.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
CHANGELOG.md (5)
9-9: Nit: rephrase for clarity
The phrase “the enable auto-joining service” is awkward. Consider rewording to “the auto-joining service” or “enable-auto-joining service” for grammatical correctness.🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: The verb ‘enable’ does not usually follow articles like ‘the’. Check that ‘enable’ is spelled correctly; using ‘enable’ as a noun may be non-standard.
Context: ...ync - Replace the device-add service by the enable auto-joining service - Remove device-re...(A_INFINITIVE)
11-11: Clarifyvia_device = Stickphrasing
The clause “add via_device = Stick” could be misread. Consider “setvia_devicetoStick” or “associate devices via the Stick”.
14-14: Mark untested features for follow-up
You note “initial support…(untested)”; consider opening an issue or TODO describing test plan for Scan and Sense devices.
16-16: Vague “Improve test-code”—suggest elaboration
This entry could benefit from specifics (e.g., added fixtures, new coverage thresholds). Consider detailing key improvements or linking to test PRs.
19-19: Remove or track TODO outside changelog
A TODO inside the changelog may be overlooked. Consider moving this note to a GitHub issue or project board and remove it from the release notes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~9-~9: The verb ‘enable’ does not usually follow articles like ‘the’. Check that ‘enable’ is spelled correctly; using ‘enable’ as a noun may be non-standard.
Context: ...ync - Replace the device-add service by the enable auto-joining service - Remove device-re...
(A_INFINITIVE)
🔇 Additional comments (7)
CHANGELOG.md (7)
5-5: Confirm version header consistency
Ensure that the new### v0.50.0header aligns with the version declared inmanifest.json,pyproject.tomlorsetup.py.
7-7: Verify dependency bump in manifest
This entry links toplugwise_usb v0.40.0. Please confirm thatmanifest.json(or equivalent) has been updated to requireplugwise_usb>=0.40.0.
8-8: Async refactor noted—looks good
Reworking the custom component to use async patterns is consistent with the PR objectives.
10-10: Removal of device-remove service—approved
Switching to the UI DELETE button for removals streamlines operations; changelog entry is clear.
12-12: HA Core improvement link included—approved
The reference to the Home Assistant Core PR enhances traceability.
13-13: Support for energy production devices—good
Adding energy production support aligns with feature goals.
15-15: Confirm Python version bump across CI
Bumping to Python 3.13 requires updating CI workflows and dev containers. Please verify that all test matrices now include 3.13 and drop unsupported versions as needed.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
custom_components/plugwise_usb/sensor.py (1)
221-242: Consider defensive programming for coordinator data access.The coordinator update handler properly checks for data existence but could benefit from additional safety measures for attribute access.
Consider adding a try-catch for the
getattrcalls to handle potential missing attributes gracefully:@callback def _handle_coordinator_update(self) -> None: """Handle updated data from the coordinator.""" data = self.coordinator.data.get(self.entity_description.node_feature, None) if data is None: _LOGGER.debug( "No %s sensor data for %s", self.entity_description.node_feature, self._node_info.mac, ) return - self._attr_native_value = getattr( - self.coordinator.data[self.entity_description.node_feature], - self.entity_description.key, - ) + try: + self._attr_native_value = getattr(data, self.entity_description.key) + except AttributeError: + _LOGGER.debug("Missing attribute %s for sensor %s", + self.entity_description.key, self._node_info.mac) + return + if self.entity_description.node_feature == NodeFeature.ENERGY: - self._attr_last_reset = getattr( - self.coordinator.data[self.entity_description.node_feature], - f"{self.entity_description.key}_reset", - ) + try: + self._attr_last_reset = getattr(data, f"{self.entity_description.key}_reset") + except AttributeError: + _LOGGER.debug("Missing reset attribute for energy sensor %s", + self._node_info.mac) self.async_write_ha_state()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)custom_components/plugwise_usb/__init__.py(1 hunks)custom_components/plugwise_usb/binary_sensor.py(1 hunks)custom_components/plugwise_usb/const.py(2 hunks)custom_components/plugwise_usb/number.py(1 hunks)custom_components/plugwise_usb/sensor.py(1 hunks)custom_components/plugwise_usb/switch.py(1 hunks)pyproject.toml(4 hunks)tests/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/init.py
- pyproject.toml
- CHANGELOG.md
- custom_components/plugwise_usb/const.py
- custom_components/plugwise_usb/number.py
🔇 Additional comments (11)
custom_components/plugwise_usb/sensor.py (3)
38-43: Well-structured entity description dataclass.The
PlugwiseSensorEntityDescriptiondataclass effectively combines the base Plugwise USB entity description with Home Assistant's sensor description, providing a clean abstraction for sensor entity metadata.
45-162: Comprehensive sensor type definitions with proper metadata.The
SENSOR_TYPEStuple provides detailed sensor configurations with appropriate device classes, state classes, units, and precision settings. The entity registry enabled defaults help manage UI clutter by disabling diagnostic sensors by default.
172-191: Robust event-driven entity creation logic.The async sensor addition logic properly filters for
NodeEvent.LOADEDevents and safely handles missing node coordinators. The feature-based filtering ensures only supported sensors are created for each node.custom_components/plugwise_usb/binary_sensor.py (1)
28-45: Clean binary sensor entity description design.The dataclass design with the
api_attributefield provides clear mapping between entity descriptions and the actual data attributes, making the entity implementation more maintainable.custom_components/plugwise_usb/switch.py (2)
28-63: Well-designed switch entity descriptions with dynamic function mapping.The entity descriptions effectively combine metadata with dynamic method resolution through the
async_switch_fnfield, enabling flexible switch control while maintaining clean configuration.
124-127: Efficient function caching in entity initialization.Caching the async switch function during initialization avoids repeated
getattrlookups during state changes, improving performance for frequent switch operations.custom_components/plugwise_usb/__init__.py (5)
50-76: Robust stick initialization with proper error handling.The stick initialization properly handles connection failures and creates device registry entries with comprehensive metadata. The use of
ConfigEntryNotReadyfor connection failures allows Home Assistant to retry setup appropriately.
80-96: Excellent event-driven node discovery implementation.The async node discovery callback properly filters events, creates coordinators for each node, and loads them asynchronously. This provides a clean separation between discovery and loading phases.
113-127: Well-structured service implementation with proper error handling.The auto-join service properly validates preconditions and provides clear error messages. The error handling converts internal exceptions to
HomeAssistantErrorfor proper user feedback.
176-203: Robust device removal implementation.The device removal function properly validates removability, handles unregistration errors gracefully, and provides appropriate logging for both success and failure cases.
216-229:Details
❌ Incorrect review comment
Verify entity migration mappings are correct.
The entity migration logic appears to revert previous migrations by mapping new suffixes back to old ones. Ensure this is intentional and aligns with the integration's entity ID strategy.
The migration mappings seem to be reversing previous changes (e.g., "last_second" back to "power_1s"). Please confirm this is the intended direction for the migration:
🏁 Script executed:
#!/bin/bash # Check if there are any references to the old entity IDs in the codebase echo "Checking for old entity ID patterns..." rg -n "power_1s|power_8s|energy_consumption_today|RSSI_in|RSSI_out" --type py echo "Checking for new entity ID patterns..." rg -n "last_second|last_8_seconds|day_consumption|rssi_in|rssi_out" --type pyLength of output: 2006
Ignore erroneous migration mapping concern
The migration logic correctly maps old suffixes to new ones (e.g.,
"power_1s"→"last_second"), matching the keys defined in sensor.py. No reversal is happening—this is intentional and aligns with the integration’s naming strategy.Likely an incorrect or invalid review comment.
| if self.coordinator.data[self.entity_description.node_feature] is None: | ||
| _LOGGER.info( | ||
| "No binary sensor data for %s", | ||
| str(self.entity_description.node_feature), | ||
| ) | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant data check in coordinator update handler.
The method performs two similar checks for data existence. The second check is redundant since data was already validated to be non-None.
Apply this diff to remove the redundant check:
@callback
def _handle_coordinator_update(self) -> None:
"""Handle updated data from the coordinator."""
data = self.coordinator.data.get(self.entity_description.node_feature, None)
if data is None:
_LOGGER.debug(
"No %s binary sensor data for %s",
self.entity_description.node_feature,
self._node_info.mac,
)
return
- if self.coordinator.data[self.entity_description.node_feature] is None:
- _LOGGER.info(
- "No binary sensor data for %s",
- str(self.entity_description.node_feature),
- )
- return
self._attr_is_on = getattr(
data,
self.entity_description.api_attribute,
)
self.async_write_ha_state()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.coordinator.data[self.entity_description.node_feature] is None: | |
| _LOGGER.info( | |
| "No binary sensor data for %s", | |
| str(self.entity_description.node_feature), | |
| ) | |
| return | |
| @callback | |
| def _handle_coordinator_update(self) -> None: | |
| """Handle updated data from the coordinator.""" | |
| data = self.coordinator.data.get(self.entity_description.node_feature, None) | |
| if data is None: | |
| _LOGGER.debug( | |
| "No %s binary sensor data for %s", | |
| self.entity_description.node_feature, | |
| self._node_info.mac, | |
| ) | |
| return | |
| self._attr_is_on = getattr( | |
| data, | |
| self.entity_description.api_attribute, | |
| ) | |
| self.async_write_ha_state() |
🤖 Prompt for AI Agents
In custom_components/plugwise_usb/binary_sensor.py around lines 113 to 118,
remove the redundant check for None on
self.coordinator.data[self.entity_description.node_feature] since the data
presence is already validated earlier. Simplify the method by deleting this
repeated conditional check and its associated logging and return statements to
avoid unnecessary duplication.
| async def async_turn_on(self, **kwargs): | ||
| """Turn the switch on.""" | ||
| self._attr_is_on = await self.async_switch_fn(True) | ||
| self.async_write_ha_state() | ||
|
|
||
| def turn_on(self, **kwargs): | ||
| """Instruct the switch to turn on.""" | ||
| setattr(self._node, self.entity_description.state_request_method, True) | ||
| async def async_turn_off(self, **kwargs): | ||
| """Turn the switch off.""" | ||
| self._attr_is_on = await self.async_switch_fn(False) | ||
| self.async_write_ha_state() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling to switch operations.
The async switch methods directly call the cached function but don't handle potential exceptions that could occur during the switch operation.
Add error handling to the switch methods:
async def async_turn_on(self, **kwargs):
"""Turn the switch on."""
- self._attr_is_on = await self.async_switch_fn(True)
- self.async_write_ha_state()
+ try:
+ self._attr_is_on = await self.async_switch_fn(True)
+ self.async_write_ha_state()
+ except Exception as exc:
+ _LOGGER.error("Failed to turn on switch %s: %s", self.entity_id, exc)
+ raise
async def async_turn_off(self, **kwargs):
"""Turn the switch off."""
- self._attr_is_on = await self.async_switch_fn(False)
- self.async_write_ha_state()
+ try:
+ self._attr_is_on = await self.async_switch_fn(False)
+ self.async_write_ha_state()
+ except Exception as exc:
+ _LOGGER.error("Failed to turn off switch %s: %s", self.entity_id, exc)
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def async_turn_on(self, **kwargs): | |
| """Turn the switch on.""" | |
| self._attr_is_on = await self.async_switch_fn(True) | |
| self.async_write_ha_state() | |
| def turn_on(self, **kwargs): | |
| """Instruct the switch to turn on.""" | |
| setattr(self._node, self.entity_description.state_request_method, True) | |
| async def async_turn_off(self, **kwargs): | |
| """Turn the switch off.""" | |
| self._attr_is_on = await self.async_switch_fn(False) | |
| self.async_write_ha_state() | |
| async def async_turn_on(self, **kwargs): | |
| """Turn the switch on.""" | |
| try: | |
| self._attr_is_on = await self.async_switch_fn(True) | |
| self.async_write_ha_state() | |
| except Exception as exc: | |
| _LOGGER.error("Failed to turn on switch %s: %s", self.entity_id, exc) | |
| raise | |
| async def async_turn_off(self, **kwargs): | |
| """Turn the switch off.""" | |
| try: | |
| self._attr_is_on = await self.async_switch_fn(False) | |
| self.async_write_ha_state() | |
| except Exception as exc: | |
| _LOGGER.error("Failed to turn off switch %s: %s", self.entity_id, exc) | |
| raise |
🤖 Prompt for AI Agents
In custom_components/plugwise_usb/switch.py around lines 144 to 152, the
async_turn_on and async_turn_off methods call async_switch_fn without handling
exceptions, which could cause unhandled errors during switch operations. Add
try-except blocks around the await calls to async_switch_fn to catch and handle
potential exceptions gracefully, ensuring the switch state is only updated if
the operation succeeds and logging or managing errors appropriately.
| while True: | ||
| await asyncio.sleep(1) | ||
| if api_stick.network_discovered: | ||
| break | ||
| # Enable/disable automatic joining of available devices when the network is up | ||
| if config_entry.pref_disable_new_entities: | ||
| _LOGGER.debug("Configuring Circle + NOT to accept any new join requests") | ||
| try: | ||
| await api_stick.set_accept_join_request(False) | ||
| except StickError as exc: | ||
| raise HomeAssistantError(f"Disabling auto-joining failed: {exc}") from exc | ||
| else: | ||
| _LOGGER.debug("Configuring Circle + to automatically accept new join requests") | ||
| try: | ||
| await api_stick.set_accept_join_request(True) | ||
| except StickError as exc: | ||
| raise HomeAssistantError(f"Enabling auto-joining failed: {exc}") from exc | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider timeout for network discovery wait loop.
The infinite loop waiting for network discovery could potentially hang if the network is never discovered. Adding a timeout would improve robustness.
Add a timeout to the network discovery wait:
+ import time
+ discovery_start = time.time()
+ discovery_timeout = 300 # 5 minutes
+
while True:
await asyncio.sleep(1)
+ if time.time() - discovery_start > discovery_timeout:
+ await api_stick.disconnect()
+ raise ConfigEntryNotReady("Network discovery timed out after 5 minutes")
if api_stick.network_discovered:
break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while True: | |
| await asyncio.sleep(1) | |
| if api_stick.network_discovered: | |
| break | |
| # Enable/disable automatic joining of available devices when the network is up | |
| if config_entry.pref_disable_new_entities: | |
| _LOGGER.debug("Configuring Circle + NOT to accept any new join requests") | |
| try: | |
| await api_stick.set_accept_join_request(False) | |
| except StickError as exc: | |
| raise HomeAssistantError(f"Disabling auto-joining failed: {exc}") from exc | |
| else: | |
| _LOGGER.debug("Configuring Circle + to automatically accept new join requests") | |
| try: | |
| await api_stick.set_accept_join_request(True) | |
| except StickError as exc: | |
| raise HomeAssistantError(f"Enabling auto-joining failed: {exc}") from exc | |
| import time | |
| discovery_start = time.time() | |
| discovery_timeout = 300 # 5 minutes | |
| while True: | |
| await asyncio.sleep(1) | |
| if time.time() - discovery_start > discovery_timeout: | |
| await api_stick.disconnect() | |
| raise ConfigEntryNotReady("Network discovery timed out after 5 minutes") | |
| if api_stick.network_discovered: | |
| break | |
| # Enable/disable automatic joining of available devices when the network is up | |
| if config_entry.pref_disable_new_entities: | |
| _LOGGER.debug("Configuring Circle + NOT to accept any new join requests") | |
| try: | |
| await api_stick.set_accept_join_request(False) | |
| except StickError as exc: | |
| raise HomeAssistantError(f"Disabling auto-joining failed: {exc}") from exc | |
| else: | |
| _LOGGER.debug("Configuring Circle + to automatically accept new join requests") | |
| try: | |
| await api_stick.set_accept_join_request(True) | |
| except StickError as exc: | |
| raise HomeAssistantError(f"Enabling auto-joining failed: {exc}") from exc |
🤖 Prompt for AI Agents
In custom_components/plugwise_usb/__init__.py around lines 136 to 153, the
infinite loop waiting for api_stick.network_discovered can hang indefinitely if
the network is never discovered. To fix this, implement a timeout mechanism by
tracking elapsed time within the loop and breaking with an error or fallback if
the timeout is exceeded. This ensures the wait does not block forever and
improves robustness.
Releasing the fully async-reworked version, linking to plugwise_usb v0.40.0.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Refactor
Tests