Fix for plugwise-beta issue 833 #706
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 17 minutes and 35 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 (1)
WalkthroughThis pull request updates the version number from v1.7.1 to v1.7.2 in both the CHANGELOG and pyproject.toml, including a new bugfix entry. It also ensures that XML-parsed data is explicitly converted to strings in the initialization module and refines the switch state methods in the legacy API by updating docstrings and adjusting parameter handling. Additionally, the test suite has been updated: new POST routes were added for the appliances endpoint, and a test was modified to pass an additional model parameter for switch-lock functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Handler
Client->>Router: POST /CORE_APPLIANCES_TAIL
alt stretch is true
Router->>Handler: Dispatch to smile_http_ok
Handler->>Client: Return HTTP 200 OK
else broken/timeout is true
Router->>Handler: Dispatch to smile_timeout
Handler->>Client: Return timeout response
end
sequenceDiagram
participant Client
participant LegacyAPI
participant SwitchController
Client->>LegacyAPI: set_switch_state(appl_id, members, model, state)
alt Individual switch request
LegacyAPI->>SwitchController: Send update for a single switch
else Group switch request
LegacyAPI->>SwitchController: Iterate and call _set_groupswitch_member_state(data, members, state, switch)
else Lock switch request
LegacyAPI->>SwitchController: Apply lock logic and update switch state
end
SwitchController-->>LegacyAPI: Acknowledgment
LegacyAPI-->>Client: Response confirmation
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (1)
plugwise/legacy/smile.py (1)
247-260: Consider using a template string for XML construction.The XML construction could be simplified using a template string for better readability.
- data = ( - f'<appliances><appliance id="{appl_id}"><name><![CDATA[{appl_name}]]></name>' - f"<description><![CDATA[]]></description><type><![CDATA[{appl_type}]]></type>" - f"<{switch.actuator}><{switch.func_type}><lock>{state}</lock></{switch.func_type}></{switch.actuator}>" - "</appliance></appliances>" - ) + data = f''' + <appliances> + <appliance id="{appl_id}"> + <name><![CDATA[{appl_name}]]></name> + <description><![CDATA[]]></description> + <type><![CDATA[{appl_type}]]></type> + <{switch.actuator}> + <{switch.func_type}> + <lock>{state}</lock> + </{switch.func_type}> + </{switch.actuator}> + </appliance> + </appliances>'''.strip()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)plugwise/__init__.py(2 hunks)plugwise/legacy/smile.py(2 hunks)pyproject.toml(1 hunks)tests/test_init.py(1 hunks)tests/test_legacy_stretch.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- CHANGELOG.md
🔇 Additional comments (7)
tests/test_legacy_stretch.py (1)
77-80: LGTM! Test case added for switch-lock functionality.The test case verifies the switch-lock functionality by calling
tinker_switchwithmodel="lock"parameter, enhancing test coverage.plugwise/legacy/smile.py (2)
234-239: LGTM! Clear and comprehensive docstring.The docstring clearly explains the different types of switches and their handling.
262-267: LGTM! Clean refactoring of data handling.The data construction is now cleanly separated before being passed to the helper method.
tests/test_init.py (2)
170-171: LGTM! Added routes for successful POST requests.The new routes properly handle POST requests to the appliances endpoint for the switch-lock functionality.
173-174: LGTM! Added timeout test routes.The timeout test routes ensure proper error handling for POST requests.
plugwise/__init__.py (2)
310-310: LGTM! Consistent type handling across legacy device detection.The explicit string conversion maintains consistency with the previous change, ensuring uniform type handling of XML-parsed data across different device types.
299-299:Details
✅ Verification successful
LGTM! Explicit string conversion adds type safety.
The explicit string conversion ensures consistent type handling of XML-parsed data.
Let's verify if there are any other XML text attributes that might benefit from explicit string conversion:
🏁 Script executed:
#!/bin/bash # Description: Find XML text attribute access patterns in the codebase # Look for potential inconsistencies in string handling of XML data # Search for direct .text access on XML elements rg -A 2 '\.text\s*[^=]*$' --type py # Search for explicit str() conversions of XML text rg -A 2 'str\([^)]*\.text\)' --type pyLength of output: 12749
Explicit Conversion Approved – Consistent Type Safety in XML Parsing
The explicit use of
str()for converting the XML data in legacy device detection (line 299 inplugwise/__init__.py) is sound and improves type safety. This defensive programming practice helps prevent potential issues with inconsistent data types.
- Location:
plugwise/__init__.py, Line 299
- The conversion
return_model = str(system.find("./gateway/product").text)ensures that the returned value is a string.- Additional Note:
- The repository shows multiple XML text accesses that are not explicitly converted. If those values are guaranteed to be strings, this is acceptable. However, you might consider reviewing them to ensure consistency in type handling in case any ambiguity arises later.
CHANGELOG.md
Outdated
| ## v.1.7.1 | ||
| ## v1.7.2 | ||
|
|
||
| - Bugfix for Plugwise-beta Issue [#833](https://github.com/plugwise/plugwise-beta/issues/833) |
There was a problem hiding this comment.
I'd suggest we mention it's not only beta - but rather solving an issue with the stretch? + leave the hash out :)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #706 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3408 3418 +10
=========================================
+ Hits 3408 3418 +10 ☔ View full report in Codecov by Sentry. |
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)plugwise/legacy/smile.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 Ruff (0.8.2)
plugwise/legacy/smile.py
253-253: SyntaxError: Invalid assignment target
258-258: SyntaxError: Expected an expression
259-259: SyntaxError: Invalid assignment target
272-272: SyntaxError: Unexpected indentation
276-276: SyntaxError: unindent does not match any outer indentation level
277-277: SyntaxError: Unexpected indentation
🪛 GitHub Check: CodeFactor
plugwise/legacy/smile.py
[notice] 256-256: plugwise/legacy/smile.py#L256
Multiple spaces after operator. (E222)
[notice] 258-258: plugwise/legacy/smile.py#L258
Multiple spaces after operator. (E222)
[notice] 255-255: plugwise/legacy/smile.py#L255
Multiple spaces after operator. (E222)
[notice] 254-254: plugwise/legacy/smile.py#L254
Multiple spaces after operator. (E222)
[notice] 257-257: plugwise/legacy/smile.py#L257
Multiple spaces after operator. (E222)
🪛 GitHub Actions: Latest commit
plugwise/legacy/smile.py
[error] 253-253: SyntaxError: Invalid assignment target
[error] 258-258: SyntaxError: Expected an expression
[error] 259-259: SyntaxError: Invalid assignment target
[error] 272-272: SyntaxError: Unexpected indentation
[error] 276-276: SyntaxError: unindent does not match any outer indentation level
[error] 277-277: SyntaxError: Unexpected indentation
[error] 294-294: SyntaxError: unindent does not match any outer indentation level
[error] 305-305: SyntaxError: unindent does not match any outer indentation level
[error] 325-325: SyntaxError: unindent does not match any outer indentation level
[error] 333-333: SyntaxError: Expected dedent, found end of file
🔇 Additional comments (2)
plugwise/legacy/smile.py (2)
234-239: LGTM! Clear and informative docstring.The docstring now effectively documents the three distinct cases handled by this method.
282-292: LGTM! Well-implemented individual switch handling.The code properly checks the lock state before switching a relay and includes appropriate error handling.
| # Handle switch-lock | ||
| if model == "lock": | ||
| state = "false" if state == "off" else "true" | ||
| appliance = self._appliances.find(f'appliance[@id="{appl_id}"]') | ||
| appl_name = appliance.find("name").text | ||
| appl_type = appliance.find("type").text | ||
| - data = ( | ||
| - f'<appliances><appliance id="{appl_id}"><name><![CDATA[{appl_name}]]></name>' | ||
| - f"<description><![CDATA[]]></description><type><![CDATA[{appl_type}]]></type>" | ||
| - f"<{switch.actuator}><{switch.func_type}><lock>{state}</lock></{switch.func_type}></{switch.actuator}>" | ||
| - "</appliance></appliances>" | ||
| - ) | ||
| + data = f''' | ||
| + <appliances> | ||
| + <appliance id="{appl_id}"> | ||
| + <name><![CDATA[{appl_name}]]></name> | ||
| + <description><![CDATA[]]></description> | ||
| + <type><![CDATA[{appl_type}]]></type> | ||
| + <{switch.actuator}> | ||
| + <{switch.func_type}> | ||
| + <lock>{state}</lock> | ||
| + </{switch.func_type}> | ||
| + </{switch.actuator}> | ||
| + </appliance> | ||
| + </appliances>'''.strip() | ||
| await self.call_request(APPLIANCES, method="post", data=data) | ||
| return | ||
|
|
There was a problem hiding this comment.
Fix syntax errors in XML data construction.
The XML data construction has syntax errors and indentation issues that are causing pipeline failures.
Apply this diff to fix the syntax and formatting:
- data = (
- f'<appliances><appliance id="{appl_id}"><name><![CDATA[{appl_name}]]></name>'
- f"<description><![CDATA[]]></description><type><![CDATA[{appl_type}]]></type>"
- f"<{switch.actuator}><{switch.func_type}><lock>{state}</lock></{switch.func_type}></{switch.actuator}>"
- "</appliance></appliances>"
- )
+ data = (
+ f'<appliances><appliance id="{appl_id}">'
+ f'<name><![CDATA[{appl_name}]]></name>'
+ f'<description><![CDATA[]]></description>'
+ f'<type><![CDATA[{appl_type}]]></type>'
+ f'<{switch.actuator}>'
+ f'<{switch.func_type}>'
+ f'<lock>{state}</lock>'
+ f'</{switch.func_type}>'
+ f'</{switch.actuator}>'
+ '</appliance></appliances>'
+ )📝 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.
| # Handle switch-lock | |
| if model == "lock": | |
| state = "false" if state == "off" else "true" | |
| appliance = self._appliances.find(f'appliance[@id="{appl_id}"]') | |
| appl_name = appliance.find("name").text | |
| appl_type = appliance.find("type").text | |
| - data = ( | |
| - f'<appliances><appliance id="{appl_id}"><name><![CDATA[{appl_name}]]></name>' | |
| - f"<description><![CDATA[]]></description><type><![CDATA[{appl_type}]]></type>" | |
| - f"<{switch.actuator}><{switch.func_type}><lock>{state}</lock></{switch.func_type}></{switch.actuator}>" | |
| - "</appliance></appliances>" | |
| - ) | |
| + data = f''' | |
| + <appliances> | |
| + <appliance id="{appl_id}"> | |
| + <name><![CDATA[{appl_name}]]></name> | |
| + <description><![CDATA[]]></description> | |
| + <type><![CDATA[{appl_type}]]></type> | |
| + <{switch.actuator}> | |
| + <{switch.func_type}> | |
| + <lock>{state}</lock> | |
| + </{switch.func_type}> | |
| + </{switch.actuator}> | |
| + </appliance> | |
| + </appliances>'''.strip() | |
| await self.call_request(APPLIANCES, method="post", data=data) | |
| return | |
| # Handle switch-lock | |
| if model == "lock": | |
| state = "false" if state == "off" else "true" | |
| appliance = self._appliances.find(f'appliance[@id="{appl_id}"]') | |
| appl_name = appliance.find("name").text | |
| appl_type = appliance.find("type").text | |
| data = ( | |
| f'<appliances><appliance id="{appl_id}">' | |
| f'<name><![CDATA[{appl_name}]]></name>' | |
| f'<description><![CDATA[]]></description>' | |
| f'<type><![CDATA[{appl_type}]]></type>' | |
| f'<{switch.actuator}>' | |
| f'<{switch.func_type}>' | |
| f'<lock>{state}</lock>' | |
| f'</{switch.func_type}>' | |
| f'</{switch.actuator}>' | |
| '</appliance></appliances>' | |
| ) | |
| await self.call_request(APPLIANCES, method="post", data=data) | |
| return |
🧰 Tools
🪛 Ruff (0.8.2)
253-253: SyntaxError: Invalid assignment target
258-258: SyntaxError: Expected an expression
259-259: SyntaxError: Invalid assignment target
272-272: SyntaxError: Unexpected indentation
🪛 GitHub Check: CodeFactor
[notice] 256-256: plugwise/legacy/smile.py#L256
Multiple spaces after operator. (E222)
[notice] 258-258: plugwise/legacy/smile.py#L258
Multiple spaces after operator. (E222)
[notice] 255-255: plugwise/legacy/smile.py#L255
Multiple spaces after operator. (E222)
[notice] 254-254: plugwise/legacy/smile.py#L254
Multiple spaces after operator. (E222)
[notice] 257-257: plugwise/legacy/smile.py#L257
Multiple spaces after operator. (E222)
🪛 GitHub Actions: Latest commit
[error] 253-253: SyntaxError: Invalid assignment target
[error] 258-258: SyntaxError: Expected an expression
[error] 259-259: SyntaxError: Invalid assignment target
[error] 272-272: SyntaxError: Unexpected indentation
Summary by CodeRabbit