Set_switch: don't raise eror when locked #755
Conversation
|
""" WalkthroughA new group switch device and group entity were added to test and fixture data, linking two existing devices. The switch state logic was refactored across multiple modules to return a boolean indicating success, with new handling for locked relays and group switches. Constants and tests were updated accordingly, and the project version incremented. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Smile as Smile API
participant Legacy as SmileLegacyAPI
participant Device as Device/Relay
Test->>Smile: set_switch_state(appl_id, members, model, state)
alt Modern API
Smile->>Smile: Determine requested_state (state == STATE_ON)
alt Model is "lock"
Smile->>Smile: Set lock state
Smile-->>Test: Return requested_state
else Model is "switchgroup"
Smile->>Smile: _set_groupswitch_member_state(...)
Smile-->>Test: Return result (bool)
else Individual relay
Smile->>Smile: Check if relay is locked
alt Locked
Smile-->>Test: Return current state (no change)
else
Smile->>Device: Set relay state
Smile-->>Test: Return requested_state
end
end
else Legacy API
Test->>Legacy: set_switch_state(...)
Legacy->>Legacy: Similar logic as Smile
Legacy-->>Test: Return result (bool)
end
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)plugwise/smile.py📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🧪 Generate Unit Tests
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #755 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3410 3450 +40
=========================================
+ Hits 3410 3450 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Nice! |
|
@coderabbitai summary |
|
Tip For best results, initiate chat on the files or code changes. @CoMPaTech CodeRabbit will now:
Please give me a moment while I scan the diff. I’ll report back shortly with the results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
plugwise/smile.py (2)
379-406:⚠️ Potential issue
requested_statebecomes wrong for the lock modelFor the
lockbranchstateis rewritten from"on"→"true"/"off"→"false".
requested_state = state == STATE_ONis calculated before that rewrite, so for single appliances everything is fine.
However, when_set_groupswitch_member_stateis called, the altered"true"/"false"value is forwarded andrequested_stateinside the helper is alwaysFalse.
Consequently a successful group-lock switch to on will reportFalse.- requested_state = state == STATE_ON + # consider "true" as ON for lock-type actuators + requested_state = state in (STATE_ON, "true")Apply the same logic in
set_switch_stateafter the lock-conversion or pass an explicit boolean to the helper.
430-455: 🛠️ Refactor suggestionHelper duplicates ON-detection bug & can mis-report outcome
See previous comment:
requested_stateshould treat"true"as on for lock actuators to avoid inverted return values.- requested_state = state == STATE_ON + requested_state = state in (STATE_ON, "true")Without this, calling code may think the switch failed while it actually succeeded.
♻️ Duplicate comments (1)
fixtures/m_adam_multiple_devices_per_zone/data.json (1)
534-546: Duplicate of previous comment – same missing attributesThe same concerns raised for the
e8ef2a01ed3b4139a53bf749204fe6b4object apply here as well.
🧹 Nitpick comments (5)
plugwise/constants.py (1)
26-26: Inconsistent on/off naming; consider adding a matchingSTATE_OFFor harmonising namesYou now have
STATE_ONalongside the pre-existing plain string constantOFF.
For readability and discoverability it is usually preferable to expose symmetrical constants (STATE_ON/STATE_OFF) or stick to one naming scheme (ON/OFF).-STATE_ON: Final = "on" +STATE_ON: Final = "on" +STATE_OFF: Final = "off" # keeps the pair symmetricAlternatively, rename
OFF→STATE_OFF(and update usages) to avoid duplication.tests/test_init.py (1)
687-699: Hard-coded state strings – reuse the new constant instead
convert = {"on": True, "off": False}duplicates knowledge that already lives inplugwise.constants.STATE_ON.- convert = {"on": True, "off": False} + convert = {pw_constants.STATE_ON: True, "off": False}This keeps the test in sync if the constant ever changes (unlikely, but still one source of truth).
userdata/adam_multiple_devices_per_zone/core.domain_objects.xml (2)
1428-1430: Ensure group reference ID and attribute quoting are consistent
Confirm that the referenced group ID (e8ef2a01ed3b4139a53bf749204fe6b4) matches the new<group>definition, and switch theidattribute to single quotes to align with the rest of the file’s style.
3151-3153: Ensure group reference ID and attribute quoting are consistent
As above, verify thisgroup id="e8ef2a01ed3b4139a53bf749204fe6b4"matches the defined group entity and replace double quotes with single quotes for consistency.plugwise/legacy/smile.py (1)
299-300: Minor duplication
requested_stateis recomputed in_set_groupswitch_member_state; you could
pass the boolean from the caller to avoid repetition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
fixtures/adam_multiple_devices_per_zone/data.json(1 hunks)fixtures/m_adam_multiple_devices_per_zone/data.json(1 hunks)plugwise/__init__.py(1 hunks)plugwise/constants.py(1 hunks)plugwise/legacy/smile.py(6 hunks)plugwise/smile.py(5 hunks)pyproject.toml(1 hunks)tests/data/adam/adam_multiple_devices_per_zone.json(1 hunks)tests/test_adam.py(2 hunks)tests/test_init.py(1 hunks)userdata/adam_multiple_devices_per_zone/core.domain_objects.xml(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
plugwise/__init__.py (1)
plugwise/smile.py (1)
set_switch_state(379-428)
tests/test_init.py (2)
plugwise/smile.py (1)
set_switch_state(379-428)plugwise/__init__.py (1)
set_switch_state(399-410)
plugwise/smile.py (1)
plugwise/legacy/smile.py (2)
call_request(332-339)_set_groupswitch_member_state(292-310)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check commit
🔇 Additional comments (5)
tests/data/adam/adam_multiple_devices_per_zone.json (1)
570-582:Details
✅ Verification successful
Missing
dev_classvalidation for “switchgroup”Introducing the synthetic device uses
dev_class: "switching".
Make sure"switching"is present in the allowed constant tuples (ACTUATOR_CLASSES,SWITCH_GROUP_TYPESetc.) – otherwise production code that validates against these may raise.
🏁 Script executed:
#!/bin/bash # Verify that the constant SWITCH_GROUP_TYPES contains "switching" grep -R --line-number '"switching"' | headLength of output: 809
Validation passed:
dev_class: "switching"is already allowed
–plugwise/constants.py:370defines
SWITCH_GROUP_TYPES: ("report", "switching")
No changes required.pyproject.toml (1)
7-7: Version bump acknowledgedVersion updated to
1.7.6a2; no issues spotted.tests/test_adam.py (2)
291-291: Test expectation updated – confirm fixture synchronisation
self.entity_itemsis asserted to be375.
If any downstream fixture is forgotten during re-generation, this fragile number may drift. Make sure all*_multiple_devices_per_zonefixtures share the same new group-switch device.
307-314: Nice negative test for locked group-switchGood addition – validates the new return behaviour when all member relays are locked.
fixtures/adam_multiple_devices_per_zone/data.json (1)
543-555: Group-device entry is missing key baseline attributesEvery other appliance object in this fixture exposes at least
available,location, (often)sensors, and azigbee_mac_address.
The new switch-group entry only containsdev_class,members,model,name,switchesandvendor.If any consumer code (or tests) blindly looks up the absent keys it will raise
KeyError. The current HA integration, for example, often checksentity["available"]andentity["location"]."e8ef2a01ed3b4139a53bf749204fe6b4": { + "available": true, + "location": "00000000000000000000000000000000", "dev_class": "switching",Please verify whether the missing attributes are intentionally omitted for group entities; if not, add them for consistency.
| <group id='e8ef2a01ed3b4139a53bf749204fe6b4'> | ||
| <name>Test</name> | ||
| <description></description> | ||
| <type>switching</type> | ||
| <created_date>2021-12-23T08:25:07.571+01:00</created_date> | ||
| <modified_date>2023-12-22T16:29:14.088+01:00</modified_date> | ||
| <deleted_date></deleted_date> | ||
| <appliances> | ||
| <appliance id='02cf28bfec924855854c544690a609ef'/> | ||
| <appliance id='4a810418d5394b3f82727340b91ba740'/> | ||
| </appliances> |
There was a problem hiding this comment.
Group definition missing relay reference and period measurements
The new <relay_functionality> lacks a <relay id='…'/> child linking to an appliance, and each <point_log> is missing its <period>/<measurement> block. These omissions will break domain parsing of group state changes and log data.
🤖 Prompt for AI Agents
In userdata/adam_multiple_devices_per_zone/core.domain_objects.xml around lines
3524 to 3534, the <relay_functionality> element is missing a <relay id='…'/>
child that links it to an appliance, and each <point_log> element lacks the
required <period> and <measurement> child blocks. To fix this, add a <relay
id='…'/> element inside the <relay_functionality> referencing the appropriate
appliance ID, and ensure every <point_log> includes properly structured <period>
and <measurement> elements to maintain correct domain parsing of group state
changes and log data.
|
Nice progress, just wondering if/how we should handle this Core-wise (e.g. if Core wouldn't expect a return False on set_switch_state()) |
bouwew
force-pushed
the
relay-sw-improve
branch
2 times, most recently
from
b3c956a to
afe8dc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
plugwise/__init__.py (1)
8-22: Critical: Missing imports for STATE_ON and STATE_OFF constants.The constants
STATE_ONandSTATE_OFFare used in line 413 but not imported, causing undefined name errors that will result in runtime failures.Add the missing imports to the constants import block:
from plugwise.constants import ( DEFAULT_LEGACY_TIMEOUT, DEFAULT_PORT, DEFAULT_TIMEOUT, DEFAULT_USERNAME, DOMAIN_OBJECTS, LOGGER, MODULES, NONE, SMILES, + STATE_OFF, + STATE_ON, STATUS, SYSTEM, GwEntityData, ThermoLoc, )
🧹 Nitpick comments (1)
plugwise/__init__.py (1)
407-407: Remove whitespace from blank line.- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugwise/__init__.py(2 hunks)plugwise/constants.py(1 hunks)plugwise/legacy/smile.py(7 hunks)plugwise/smile.py(6 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- plugwise/constants.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
plugwise/__init__.py
[error] 363-363: line too long (86 > 79 characters)
(E501)
[error] 413-413: undefined name 'STATE_ON'
(F821)
[error] 413-413: undefined name 'STATE_OFF'
(F821)
plugwise/legacy/smile.py
[error] 238-238: line too long (83 > 79 characters)
(E501)
[error] 275-275: line too long (81 > 79 characters)
(E501)
[error] 284-284: line too long (86 > 79 characters)
(E501)
[error] 291-291: line too long (84 > 79 characters)
(E501)
[error] 295-295: line too long (82 > 79 characters)
(E501)
[error] 296-296: line too long (118 > 79 characters)
(E501)
plugwise/smile.py
[error] 313-313: line too long (85 > 79 characters)
(E501)
[error] 382-382: line too long (83 > 79 characters)
(E501)
[error] 434-434: line too long (86 > 79 characters)
(E501)
[error] 435-435: line too long (88 > 79 characters)
(E501)
[error] 442-442: line too long (84 > 79 characters)
(E501)
[error] 446-446: line too long (82 > 79 characters)
(E501)
[error] 447-447: line too long (118 > 79 characters)
(E501)
[error] 453-453: line too long (87 > 79 characters)
(E501)
🪛 Ruff (0.11.9)
plugwise/__init__.py
407-407: Blank line contains whitespace
Remove whitespace from blank line
(W293)
413-413: Undefined name STATE_ON
(F821)
413-413: Undefined name STATE_OFF
(F821)
🪛 GitHub Actions: Latest commit
plugwise/__init__.py
[warning] 407-407: Ruff W293: Blank line contains whitespace. Remove whitespace from blank line.
[error] 413-413: Ruff F821: Undefined name STATE_ON.
[error] 413-413: Ruff F821: Undefined name STATE_OFF.
🔇 Additional comments (9)
plugwise/__init__.py (2)
362-365: Good addition of preset validation logic.The validation ensures that presets exist and are valid before attempting to set them, providing clear error messages to users.
Consider breaking the long line 363 to improve readability:
- if (presets := self._presets(loc_id)) is None: - raise PlugwiseError("Plugwise: no presets available.") # pragma: no cover + presets = self._presets(loc_id) + if presets is None: + raise PlugwiseError("Plugwise: no presets available.") # pragma: no cover
405-423: Excellent enhancement to return switch state feedback.The method now provides immediate feedback on switch state changes and includes proper input validation. The boolean return value will be very useful for callers to know if the operation succeeded.
plugwise/smile.py (3)
10-28: Good addition of required imports.All necessary imports are properly added to support the new functionality.
378-439: Excellent refactoring of switch state management.The method now provides clear boolean feedback and handles edge cases properly:
- Returns current state for locked relays instead of attempting changes
- Properly delegates group switch handling to the helper method
- Uses type casting and constants for better type safety
The logic for checking locked relays (lines 432-436) is particularly well implemented.
Note: Several lines exceed the 79-character limit. Consider breaking them for better readability in future refactoring.
441-463: Well-designed group switch state management.The helper method correctly tracks successful member switches and returns appropriate boolean values. The logic ensures that if any member was successfully switched, the requested state is returned; otherwise, the current group state is maintained.
plugwise/legacy/smile.py (4)
21-22: Good addition of state constants.Proper import of
STATE_OFFandSTATE_ONconstants for consistent state handling.
195-196: Excellent use of constants for state validation.Replacing string literals with constants improves consistency and reduces potential for typos.
Also applies to: 214-215
234-288: Well-implemented boolean return logic for switch states.The method correctly:
- Returns the current state for locked relays (line 285)
- Returns the requested state after successful operations (line 288)
- Handles group switches by delegating to the helper method
This addresses the concerns from previous reviews about returning accurate state information for locked relays.
290-310: Good group switch member state tracking.The helper method properly tracks successful switches and returns appropriate boolean values. The logic ensures accurate feedback about the group's end state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugwise/smile.py (2)
381-444: Comprehensive switch state implementation with proper boolean returnsThe refactored method excellently handles:
- Type casting for switch models
- Early return for locked relays with current state
- Unified XML data construction
- Proper delegation to group switch helper
However, there are line length violations that should be addressed.
Apply this formatting fix for line length compliance:
- async def set_switch_state( - self, appl_id: str, members: list[str] | None, model: str, state: str - ) -> bool: + async def set_switch_state( + self, + appl_id: str, + members: list[str] | None, + model: str, + state: str, + ) -> bool:- return await self._set_groupswitch_member_state( - appl_id, data, members, state, switch - ) + return await self._set_groupswitch_member_state( + appl_id, data, members, state, switch + )- # Don't switch a relay when its corresponding lock-state is true or no - # lock is present. That means the relay can't be controlled by the user. + # Don't switch a relay when its corresponding lock-state is true or no + # lock is present. That means the relay can't be controlled by the user.
446-468: Robust group switch member handling with proper state trackingThe helper method correctly:
- Tracks switched members vs locked ones
- Returns requested state when any member was switched
- Returns current state when no changes occurred
- Handles locked members appropriately
Line length issues should be addressed for consistency.
Apply this formatting fix:
- async def _set_groupswitch_member_state( - self, appl_id: str, data: str, members: list[str], state: str, switch: Munch - ) -> bool: + async def _set_groupswitch_member_state( + self, + appl_id: str, + data: str, + members: list[str], + state: str, + switch: Munch, + ) -> bool:- """Helper-function for set_switch_state(). - - Set the requested state of the relevant switch within a group of switches. - Return the current group-state when none of the switches has changed its state, the requested state otherwise. + """Helper-function for set_switch_state(). + + Set the requested state of the relevant switch within a group of switches. + Return the current group-state when none of the switches has changed its + state, the requested state otherwise.plugwise/legacy/smile.py (1)
237-293: Well-implemented boolean return logic with proper locked relay handlingThe method correctly:
- Returns boolean indicating resulting state
- Handles locked relays by returning current state (addressing previous review feedback)
- Delegates group switches appropriately
- Uses consistent state constants
Line length violations should be addressed.
Apply this formatting fix for line length compliance:
- async def set_switch_state( - self, appl_id: str, members: list[str] | None, model: str, state: str - ) -> bool: + async def set_switch_state( + self, + appl_id: str, + members: list[str] | None, + model: str, + state: str, + ) -> bool:- return await self._set_groupswitch_member_state( - appl_id, data, members, state, switch - ) + return await self._set_groupswitch_member_state( + appl_id, data, members, state, switch + )- if model == "relay" and self.gw_entities[appl_id]["switches"]["lock"]: + if ( + model == "relay" + and self.gw_entities[appl_id]["switches"]["lock"] + ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugwise/__init__.py(2 hunks)plugwise/constants.py(1 hunks)plugwise/legacy/smile.py(7 hunks)plugwise/smile.py(6 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- plugwise/constants.py
🧰 Additional context used
🪛 Flake8 (7.2.0)
plugwise/smile.py
[error] 382-382: line too long (83 > 79 characters)
(E501)
[error] 419-419: line too long (84 > 79 characters)
(E501)
[error] 442-442: line too long (84 > 79 characters)
(E501)
[error] 446-446: line too long (82 > 79 characters)
(E501)
[error] 447-447: line too long (118 > 79 characters)
(E501)
[error] 453-453: line too long (87 > 79 characters)
(E501)
[error] 465-465: line too long (82 > 79 characters)
(E501)
plugwise/__init__.py
[error] 413-413: undefined name 'STATE_ON'
(F821)
[error] 413-413: undefined name 'STATE_OFF'
(F821)
plugwise/legacy/smile.py
[error] 284-284: line too long (86 > 79 characters)
(E501)
[error] 291-291: line too long (84 > 79 characters)
(E501)
[error] 295-295: line too long (82 > 79 characters)
(E501)
[error] 296-296: line too long (118 > 79 characters)
(E501)
🪛 GitHub Check: codecov/patch
plugwise/__init__.py
[warning] 412-412: plugwise/init.py#L412
Added line #L412 was not covered by tests
🪛 Ruff (0.11.9)
plugwise/__init__.py
413-413: Undefined name STATE_ON
(F821)
413-413: Undefined name STATE_OFF
(F821)
🔇 Additional comments (7)
plugwise/__init__.py (2)
18-19: LGTM: Clean constant importsThe new STATE_OFF and STATE_ON constants are properly imported and will provide consistent state representation across the codebase.
403-421: Excellent implementation with comprehensive validation and documentationThe method has been well-refactored to:
- Return a boolean indicating the resulting state
- Validate input state against allowed constants
- Provide clear documentation about return values
- Handle exceptions appropriately
The implementation correctly addresses the PR objective of returning the resulting state.
plugwise/smile.py (2)
25-28: LGTM: Proper constant and type importsThe new imports STATE_OFF, STATE_ON, and SwitchType are correctly added and will improve type safety and consistency.
315-376: LGTM: Consistent constant usage in schedule state handlingGood update to use the new STATE_OFF and STATE_ON constants instead of string literals, improving consistency across the codebase.
plugwise/legacy/smile.py (3)
21-22: LGTM: Consistent constant imports across implementationsThe STATE_OFF and STATE_ON constants are properly imported, maintaining consistency with the main SmileAPI implementation.
200-219: LGTM: Proper constant usage in schedule state validationGood update to use the new constants for validation and state assignment, improving consistency and reducing magic strings.
326-326: LGTM: Minor pragma comment fixGood housekeeping fix to the pragma comment formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugwise/smile.py (1)
387-387: Fix line length violations.Several lines exceed the 79 character limit. Consider breaking these lines for better readability.
- For switch-locks, sets the lock state using a different data format. + For switch-locks, sets the lock state using a different data format. - # Don't switch a relay when its corresponding lock-state is true or no - # lock is present. That means the relay can't be controlled by the user. + # Don't switch a relay when its corresponding lock-state is true or + # no lock is present. That means the relay can't be controlled by the user. - self, appl_id: str, data: str, members: list[str], state: str, switch: Munch + self, + appl_id: str, + data: str, + members: list[str], + state: str, + switch: Munch, - Return the current group-state when none of the switches has changed its state, the requested state otherwise. + Return the current group-state when none of the switches has changed its + state, the requested state otherwise.Also applies to: 439-440, 447-447, 451-452
plugwise/legacy/smile.py (1)
243-243: Fix line length violations for better readability.Multiple lines exceed the 79 character limit. Consider breaking these lines appropriately.
- For switch-locks, sets the lock state using a different data format. + For switch-locks, sets the lock state using a different data format. - data = f"<{switch.func_type}><state>{state}</state></{switch.func_type}>" + data = ( + f"<{switch.func_type}><state>{state}</state></{switch.func_type}>" + ) - if model == "relay" and self.gw_entities[appl_id]["switches"]["lock"]: + if ( + model == "relay" + and self.gw_entities[appl_id]["switches"]["lock"] + ): - self, appl_id: str, data: str, members: list[str], state: str, switch: Munch + self, + appl_id: str, + data: str, + members: list[str], + state: str, + switch: Munch, - Return the current group-state when none of the switches has changed its state, the requested state otherwise. + Return the current group-state when none of the switches has changed + its state, the requested state otherwise.Also applies to: 280-280, 289-289, 296-296, 300-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/__init__.py(2 hunks)plugwise/legacy/smile.py(7 hunks)plugwise/smile.py(6 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
plugwise/legacy/smile.py
[error] 243-243: line too long (83 > 79 characters)
(E501)
[error] 280-280: line too long (81 > 79 characters)
(E501)
[error] 289-289: line too long (86 > 79 characters)
(E501)
[error] 296-296: line too long (84 > 79 characters)
(E501)
[error] 300-300: line too long (82 > 79 characters)
(E501)
[error] 301-301: line too long (118 > 79 characters)
(E501)
plugwise/smile.py
[error] 318-318: line too long (85 > 79 characters)
(E501)
[error] 387-387: line too long (83 > 79 characters)
(E501)
[error] 439-439: line too long (86 > 79 characters)
(E501)
[error] 440-440: line too long (88 > 79 characters)
(E501)
[error] 447-447: line too long (84 > 79 characters)
(E501)
[error] 451-451: line too long (82 > 79 characters)
(E501)
[error] 452-452: line too long (118 > 79 characters)
(E501)
[error] 458-458: line too long (87 > 79 characters)
(E501)
🪛 GitHub Check: codecov/patch
plugwise/__init__.py
[warning] 412-412: plugwise/init.py#L412
Added line #L412 was not covered by tests
🔇 Additional comments (11)
plugwise/__init__.py (3)
18-19: LGTM - Constants properly imported for state validation.The addition of
STATE_OFFandSTATE_ONconstants improves code consistency and readability across the codebase.
403-422: Excellent implementation of boolean return logic with proper validation.The method now:
- Returns a meaningful boolean indicating the switch state result
- Validates input state against defined constants
- Provides clear documentation of the return behavior
- Maintains proper exception handling for connection failures
The implementation correctly addresses the PR objective of returning the resulting state.
412-412: Let’s check for any tests coveringset_switch_stateorPlugwiseErrorin test files:#!/bin/bash # Search for usages of set_switch_state in test files rg -n "set_switch_state" --glob "*test*.py" echo "----" # Search for usages of PlugwiseError in test files rg -n "PlugwiseError" --glob "*test*.py"plugwise/smile.py (4)
25-28: LGTM - Proper imports for enhanced type safety and constants.The addition of
STATE_OFF,STATE_ON, andSwitchTypeimports along with thecastfunction improves type safety and code consistency.
315-316: LGTM - Consistent use of state constants.Replacing string literals with
STATE_OFFandSTATE_ONconstants improves maintainability and reduces the risk of typos.Also applies to: 320-321, 373-374, 376-377
383-444: Solid implementation of boolean return logic for switch states.The implementation correctly:
- Validates and converts the model type
- Retrieves current state from gateway entities
- Handles different switch types (relay, dhw_cm_switch, cooling_ena_switch, lock)
- Delegates group switches to the helper method
- Returns current state for locked relays (preventing unnecessary operations)
- Returns requested state after successful operations
The logic is sound and handles edge cases appropriately.
446-468: Group switch logic correctly tracks state changes.The helper method properly:
- Tracks how many members were successfully switched
- Ignores locked members (preventing errors)
- Returns the requested state if any members were changed
- Returns the current group state if no changes were made
This ensures accurate group switch state reporting.
plugwise/legacy/smile.py (4)
21-22: LGTM - Consistent state constants adoption.The addition of
STATE_OFFandSTATE_ONconstants maintains consistency with the other API implementations.
200-201: LGTM - Proper state validation using constants.The schedule state validation now uses the standardized constants, improving consistency and maintainability.
Also applies to: 219-220
237-293: Excellent implementation addressing previous review concerns.The updated
set_switch_statemethod correctly:
- Returns boolean values indicating the actual switch state result
- Uses standardized state constants for validation
- Returns current state for locked relays (avoiding incorrect failure indication)
- Properly handles lock switches with the right data format
- Delegates group switches to the updated helper method
This addresses the concerns raised in previous reviews about returning incorrect values for locked relays.
295-315: Group switch logic properly tracks successful state changes.The helper method correctly:
- Only attempts to switch unlocked members
- Tracks how many members were successfully switched
- Returns the requested state if any switches were changed
- Returns the current group state when no changes were made
This ensures accurate reporting of group switch operations and handles locked members gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugwise/legacy/smile.py (2)
239-239: Update docstring to reflect boolean returnThe method signature now returns
bool, but the docstring should be updated to clearly document this change and what the boolean represents.The past review comment already addressed this issue, suggesting documentation updates for the return type.
Also applies to: 245-245
305-315: Group switch logic may need refinementSimilar to the main API, the group switch logic counts switched members rather than verifying end-states. This could return incorrect results when members are already in the desired state.
This issue was flagged in past reviews and appears to follow the same pattern as the main smile.py file.
🧹 Nitpick comments (3)
plugwise/smile.py (2)
391-393: Verify type casting safetyThe
cast(SwitchType, model)assumes the model parameter is always a validSwitchType. Consider adding validation to ensure this cast is safe.- model_type = cast(SwitchType, model) + if model not in SwitchType.__members__.values(): + raise PlugwiseError(f"Invalid switch model: {model}") + model_type = cast(SwitchType, model)
387-387: Address line length violationsMultiple lines exceed the 79-character limit. Consider breaking them for better readability:
- For switch-locks, sets the lock state using a different data format. + For switch-locks, sets the lock state using a different data format.Apply similar formatting to other long lines identified by static analysis.
Also applies to: 439-440, 447-447, 451-452, 458-458
plugwise/legacy/smile.py (1)
243-243: Fix line length violationsMultiple lines exceed the 79-character limit. Consider reformatting for better readability:
- For switch-locks, sets the lock state using a different data format. + For switch-locks, sets the lock state using a different data format.Apply similar formatting to the other long lines identified by static analysis.
Also applies to: 280-280, 289-289, 296-296, 300-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/__init__.py(2 hunks)plugwise/legacy/smile.py(7 hunks)plugwise/smile.py(6 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
plugwise/legacy/smile.py
[error] 243-243: line too long (83 > 79 characters)
(E501)
[error] 280-280: line too long (81 > 79 characters)
(E501)
[error] 289-289: line too long (86 > 79 characters)
(E501)
[error] 296-296: line too long (84 > 79 characters)
(E501)
[error] 300-300: line too long (82 > 79 characters)
(E501)
[error] 301-301: line too long (118 > 79 characters)
(E501)
plugwise/smile.py
[error] 318-318: line too long (85 > 79 characters)
(E501)
[error] 387-387: line too long (83 > 79 characters)
(E501)
[error] 439-439: line too long (86 > 79 characters)
(E501)
[error] 440-440: line too long (88 > 79 characters)
(E501)
[error] 447-447: line too long (84 > 79 characters)
(E501)
[error] 451-451: line too long (82 > 79 characters)
(E501)
[error] 452-452: line too long (118 > 79 characters)
(E501)
[error] 458-458: line too long (87 > 79 characters)
(E501)
🪛 GitHub Check: codecov/patch
plugwise/__init__.py
[warning] 412-412: plugwise/init.py#L412
Added line #L412 was not covered by tests
🔇 Additional comments (7)
plugwise/__init__.py (3)
18-19: LGTM: Clean constant importsThe addition of
STATE_OFFandSTATE_ONconstants improves code consistency and maintainability across the codebase.
404-410: Excellent documentation enhancementThe updated docstring clearly explains the boolean return values and different scenarios (on/off/unchanged when locked). This provides much better API contract clarity for consumers.
411-412: ```shell
#!/bin/bash
set -eecho "1) Looking for any tests calling set_switch_state"
rg -n "set_switch_state" tests/ || trueecho -e "\n2) Looking for any tests expecting PlugwiseError"
rg -n "PlugwiseError" tests/ || trueecho -e "\n3) Looking for any tests referencing valid states"
rg -n "STATE_OFF" tests/ || true
rg -n "STATE_ON" tests/ || true</details> <details> <summary>plugwise/smile.py (1)</summary> `437-441`: **Excellent lock handling logic** The improved lock handling correctly returns the current state when a relay is locked, rather than attempting the switch operation. This provides accurate feedback to callers about the actual device state. </details> <details> <summary>plugwise/legacy/smile.py (3)</summary> `21-22`: **Consistent constant usage** Good adoption of the new `STATE_OFF` and `STATE_ON` constants, maintaining consistency across legacy and current APIs. --- `200-200`: **Proper state validation alignment** The schedule state validation now uses the same constants as the main API, ensuring consistent behavior across legacy and current implementations. Also applies to: 219-219 --- `326-326`: **Good cleanup of extraneous character** The removal of the extra quote character in the comment improves code cleanliness. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
6-6: Standardize PR reference formatUse
[#755]instead of[755]to align with the existing changelog convention of prefixing PR references with#.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Prepare
🔇 Additional comments (1)
CHANGELOG.md (1)
3-3: Verify version bump consistencyEnsure the new header
## v1.7.6matches the version declared insetup.pyand any__init__.pyversion constants to prevent mismatches across the project.
|
bouwew
changed the title
Key change: don't raise an error when a locked switch is being toggled, this causes a misrepresentation of the switch's state in the Plugwise integration in Home Assistant. When doing nothing (return None) then the switch's state returns to the original state after being toggled in the frontend.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores