Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR replaces the existing untyped Changes
Sequence DiagramsequenceDiagram
actor Client
participant Request as Request Handler
participant Parser as XML Parser
participant Validator as Pydantic Validator
participant Storage as Data Storage
Client->>Request: Call _request(DOMAIN_OBJECTS, new=True)
Request->>Request: Fetch XML from endpoint
Request->>Parser: Pass raw response text
Parser->>Parser: Parse with defusedxml
Parser->>Parser: Convert via xmltodict to dict
Parser->>Validator: Pass dict for validation
Validator->>Validator: DomainObjects.model_validate(dict)
Validator->>Validator: Build runtime ID indexes
Validator->>Storage: Return validated models
Storage->>Storage: Assign to self.data
Storage-->>Client: Return response string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
plugwise/smilecomm.py (2)
165-176:⚠️ Potential issue | 🟠 MajorParse the escaped payload in the typed branch.
Lines 167-170 already sanitize illegal XML characters, but Line 174 reparses the original
resultstring. Any response that only works because ofescape_illegal_xml_characters()still fails oncenew=Trueis enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/smilecomm.py` around lines 165 - 176, The branch that handles new=True currently parses and sanitizes the response into the local variable xml but then calls self._parse_xml on the original result string, so responses that required escape_illegal_xml_characters() still fail; update the new=True path to pass the already-sanitized/parsed data (use the xml variable or its decoded/text form) into self._parse_xml (and into domain_objects) instead of using result, so InvalidXMLError handling and subsequent assignment to self.data use the sanitized payload.
70-131:⚠️ Potential issue | 🟠 MajorCarry
method,data, andnewthrough retries.The new typed path is dropped on retry because both retry branches recurse with only
command. After a transient failure, a domain-objects fetch falls back to the old XML path, and POST/PUT retries lose their body.Minimal fix
- return await self._request(command, retry - 1) + return await self._request( + command, + retry - 1, + method=method, + data=data, + new=new, + ) ... - return await self._request(command, retry - 1) + return await self._request( + command, + retry - 1, + method=method, + data=data, + new=new, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/smilecomm.py` around lines 70 - 131, In _request, the retry recursive calls drop method, data and new so retries reset to defaults; change the two recursive calls inside the exception handler and the 504 status branch to pass the same parameters (e.g. return await self._request(command, retry - 1, method=method, data=data, new=new)) so _request preserves the original method, data and new flags across retries (locate the recursive calls inside the _request function).plugwise/smile.py (1)
68-95: 🛠️ Refactor suggestion | 🟠 MajorMake
dataconsistently mean one model type.The signature says
PlugwiseData, but the implementation keeps treatingself.dataas the inner domain-object container (self.data.locationhere,self.data.notificationlater). Please either storedata.domain_objectsor change the contract to match the actual object. The debugprint()on Line 95 also keeps CI red.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/smile.py` around lines 68 - 95, Constructor stores a mixed object in self.data and leaves a stray print call; update __init__ so data consistently holds one model type: either assign the domain-object container (e.g., self.data = data.domain_objects) and keep the parameter type PlugwiseData, or change the parameter/type to accept the inner domain object directly and assign that to self.data (refer to __init__, self.data, and usages like self.data.location and self.data.notification), and remove the debug print("HOI16 ...") (or replace with a proper logger.debug) so CI is no longer affected.plugwise/helper.py (1)
380-389:⚠️ Potential issue | 🔴 Critical
get_appliance()returns a model, not an XML node.Lines 382-389 still route that model through XML helpers and even call
appliance.find("type"). As written, the first non-P1 appliance update will blow up in the measurement path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/helper.py` around lines 380 - 389, get_appliance() returns a domain model, not an XML node, but the code is passing that model into XML helper functions and calling appliance.find("type"), which will crash; fix by passing the original XML node (the existing variable entity) into the XML-specific helpers (_appliance_measurements, _get_lock_state, _get_toggle_state, and the actuator-type check) and only use the model from get_appliance() for places that need model properties; specifically, change calls that currently pass appliance into _appliance_measurements, _get_lock_state, _get_toggle_state and the appliance.find("type").text check to use entity instead, leaving the appliance model available for any model-based logic.plugwise/__init__.py (3)
124-130:⚠️ Potential issue | 🔴 CriticalCritical: Undefined variable
resultand incorrect boolean check.Two issues in this block:
- Line 124:
dsmrmain is Noneis incorrect sincedsmrmainis declared asboolon line 115. Should benot dsmrmain.- Line 128:
resultis undefined - it was the return value of_request()in the old code but is no longer captured.🐛 Proposed fix
- if "Plugwise" not in vendor_names and dsmrmain is None: # pragma: no cover + if "Plugwise" not in vendor_names and not dsmrmain: # pragma: no cover LOGGER.error( "Connected but expected text not returned, we got %s. Please create" " an issue on http://github.com/plugwise/python-plugwise", - result, + vendor_names, ) raise ResponseError🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/__init__.py` around lines 124 - 130, The conditional incorrectly checks "dsmrmain is None" and the error log references an undefined variable `result`; change the condition to use `not dsmrmain` (since `dsmrmain` is a bool) and capture or construct the response content to log (e.g., the variable returned by the previous `_request()` call or the current response stored in the surrounding scope) so that LOGGER.error in this block logs the actual response content; update the branch around vendor_names, dsmrmain, LOGGER.error, and the ResponseError raise to use the correct boolean check and a valid response variable.
156-166:⚠️ Potential issue | 🔴 CriticalCritical:
SmileLegacyAPIdoes not acceptdataparameter - TypeError at runtime.The
SmileLegacyAPI.__init__signature (seeplugwise/legacy/smile.py:37-58) does not include adata: PlugwiseDataparameter. Passingself.dataat line 165 will cause aTypeErrorwhen instantiating the legacy API.🐛 Proposed fix - remove data parameter for legacy
else SmileLegacyAPI( self._is_thermostat, self._loc_data, self._on_off_device, self._opentherm_device, self._request, self._stretch_v2, self._target_smile, self.smile, - self.data, )Alternatively, update
SmileLegacyAPI.__init__inplugwise/legacy/smile.pyto accept and store thedataparameter if it's needed there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/__init__.py` around lines 156 - 166, The call to SmileLegacyAPI is passing self.data which doesn't match SmileLegacyAPI.__init__ (causing a TypeError); fix by removing the trailing self.data argument from the SmileLegacyAPI(...) instantiation so the argument list matches the constructor signature used in legacy/smile.py, or alternatively update SmileLegacyAPI.__init__ to accept and store a data: PlugwiseData parameter (add parameter to the constructor and assign to an instance attribute) if the legacy class actually requires that data.
8-8:⚠️ Potential issue | 🟡 MinorRemove unused imports flagged by linter.
Pipeline failures indicate these imports are unused:
castfromtyping(line 8)MODULESfromplugwise.constants(line 17)Munchfrommunch(line 41)🧹 Proposed fix
-from typing import cast +At line 17, remove
MODULESfrom the import list. At line 41, remove theMunchimport entirely if no longer needed.Also applies to: 17-17, 41-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/__init__.py` at line 8, Remove the unused imports flagged by the linter: delete `cast` from the `from typing import ...` line, remove `MODULES` from the `from plugwise.constants import ...` import, and drop the `Munch` import entirely; search for any usages of `cast`, `MODULES`, or `Munch` in the module (they should be unused) and if none exist remove the identifiers from their import statements so the file no longer imports these symbols.plugwise/common.py (1)
154-172:⚠️ Potential issue | 🔴 CriticalInconsistent entity key usage:
appl.idvsappl.entity_idwill causeKeyError.Line 156 creates a dictionary entry keyed by
appl.id(the appliance's XML identifier), but line 172 attempts to access it usingappl.entity_id(a location-based identifier set dynamically in helper.py). Since these are different values, this will raise aKeyErrorat runtime.The fix should be to use
appl.entity_idconsistently as the dictionary key, since the codebase design (as evidenced by smartmeter handling and zone management) usesentity_idas the primary entity identifier:🐛 Proposed fix - use entity_id as the key
def _create_gw_entities(self, appl: Appliance) -> None: """Helper-function for creating/updating gw_entities.""" - self.gw_entities[appl.id] = {"dev_class": appl.type} + self.gw_entities[appl.entity_id] = {"dev_class": appl.type} self._count += 1 for key, value in { ... }.items(): if value is not None or key == "location": appl_key = cast(ApplianceType, key) self.gw_entities[appl.entity_id][appl_key] = value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/common.py` around lines 154 - 172, In _create_gw_entities, replace the initial dictionary key self.gw_entities[appl.id] = {"dev_class": appl.type} with self.gw_entities[appl.entity_id] = {"dev_class": appl.type} so the same entity identifier (appl.entity_id) is used for creation and subsequent updates; keep the rest of the loop as-is (the appl_key cast and assignments to self.gw_entities[appl.entity_id][appl_key]) so no KeyError occurs when updating entity fields.
🧹 Nitpick comments (1)
plugwise/common.py (1)
281-297: Remove or properly comment out the TODO legacy code block.This commented-out code block at lines 281-297 creates confusion. The
"""string literal afterreturn module_datawill actually be executed as a no-op expression statement, not as a comment. While harmless, it's dead code that should be handled properly.Either:
- Remove this block entirely if no longer needed, or
- Convert to a proper
# TODOcomment block if it needs to be preserved for reference🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/common.py` around lines 281 - 297, Remove or properly comment out the legacy dead-code block that follows the return module_data no-op string literal: the triple-quoted string starting before the legacy logic (which references appl_search, link_tag, xml_2, self._domain_objects, get_vendor_name, get_zigbee_data, and module_data) should either be deleted entirely or converted into line-comments prefixed with # to avoid an executable string literal; ensure no functional code remains inside that block and preserve any necessary notes as non-executable comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 17: The changelog bullet "Attempt to ditch untyped Munch for the existing
TypedDicts by leveraging pydantic to type xmltodict XML conversion" is currently
under the v1.11.3 section and should be moved back to the ongoing section; edit
CHANGELOG.md to cut that bullet from its current location (line containing that
exact text) and paste it under the "## Ongoing" heading so it renders as part of
Ongoing instead of v1.11.3.
In `@plugwise/__init__.py`:
- Around line 179-180: Remove the leftover debug print statements that output
"HOI14", "HOI11a/b" and "HOI11c" (the prints referencing self and self.smile)
from plugwise/__init__.py; locate the prints in the methods where self and
self.smile are printed and delete those print(...) calls, or replace them with
appropriate logger.debug(...) calls using the module/class logger if you need
retained diagnostics, ensuring no raw print statements remain in production
code.
- Around line 266-277: self.data.module is a list, but the flags _on_off_device
and _opentherm_device treat it like a single object; update the checks for
self._on_off_device and self._opentherm_device to iterate over the modules (e.g.
use a loop or any(...) across self.data.module) and check each module's
protocols for "on_off_boiler" and "open_therm_boiler" respectively, setting the
boolean to True if any module contains the protocol; ensure you reference
self.data.module and the attribute protocols on each module item when
implementing the fix.
- Around line 208-211: The call to _smile_detect_legacy in _smile_detect is
using undefined variables result and dsmrmain; update the code so legacy
detection receives the correct data: either add dsmrmain as a parameter to
_smile_detect and pass it from connect(), or store dsmrmain on self in connect()
and read self.dsmrmain inside _smile_detect, and replace result with the actual
XML/object the legacy routine needs (or refactor _smile_detect_legacy to accept
the current model data). Locate usages in connect(), _smile_detect(), and
_smile_detect_legacy to make the parameter/attribute consistent and ensure all
callers are updated accordingly.
In `@plugwise/common.py`:
- Around line 34-48: Remove the stray XML-style legacy block that uses
module.find("./protocols/network_router") and
module.find("./protocols/network_coordinator") (the first if legacy: block that
sets module_data["zigbee_mac_address"] via router/coord variables); keep the
subsequent model-based legacy handling that accesses
module.protocols.network_router and module.protocols.network_coordinator and
assigns module_data.zigbee_mac_address, then return—i.e., delete the XML-access
block so only the Pydantic model-style legacy logic remains.
In `@plugwise/helper.py`:
- Around line 473-478: The loop over self._domain_objects.notification fails
when DomainObjects.notification is a singleton or None; before iterating in the
block that accesses notification.id, notification.type and notification.message
and writes to self._notifications, coerce DomainObjects.notification into an
iterable: if it's None skip, if it's a single Notification wrap it in a list,
otherwise keep the list as-is; then iterate that normalized list so the logic
inside (using notification.id, notification.type, notification.message and
updating self._notifications) works for all cases.
- Around line 150-170: The P1 path still treats module_data like a dict and sets
attributes on an undefined appl, causing TypeError/NameError; update the block
that calls self._get_module_data (with MODULE_LOCATOR and tag="electricity") to
(1) check module_data.content (attribute) for existence and return early if
falsy, and (2) instantiate or obtain the appliance object before assigning
properties (mirror how other device paths create the appliance instance) and use
attribute access on module_data (e.g., module_data.firmware_version,
module_data.vendor_model, module_data.vendor_name) instead of dict indexing;
ensure appl.entity_id chooses between self._home_loc_id and self._gateway_id
using self.smile.anna_p1 as shown.
- Around line 115-120: The bug is that when appliance.location is present the
code sets appliance.fixed_location = appliance.id which maps devices to
themselves instead of to their zone; change the assignment to use the referenced
location id (e.g. appliance.fixed_location = appliance.location or
appliance.fixed_location = appliance.location.id if location is an object) so
zones keyed by location_id join correctly; leave the thermostat check and
self._home_loc_id fallback as-is.
- Around line 208-246: _appliance_info_finder mixes model-backed and XML-backed
flows: remove debug prints, stop using the nonexistent appl variable, and make
the branch calls and assignments consistently operate on the Appliance instance.
Specifically, in _appliance_info_finder use appliance (not appl) when setting
model/model_id/vendor_name/etc. for the plug branch and call
check_model(appliance.model_id, appliance.vendor_name); ensure
_appl_gateway_info and _appl_heater_central_info accept an Appliance (or adapt
the call to pass the XML element) so those helpers no longer call .find() on an
Appliance; also remove the print() debugging lines and return None (not Munch())
where appropriate for missing data in the heater/gateway flows.
- Around line 198-201: You replaced self._home_location with a single Location
model but _power_data_from_location() and _get_gateway_outdoor_temp() still
expect a collection with a .find() method; revert to keeping the original
location collection (or add a new separate attribute) so callers keep working:
change the assignment that sets self._home_location (currently using next(...))
to keep the collection (e.g., self._home_location = self.data.location) and use
its .find(loc.loc_id) where needed, or if you must keep the model also create a
new attr like self._home_location_model = next(...) and leave
self._home_location as the collection so _power_data_from_location and
_get_gateway_outdoor_temp continue to call .find() without error.
In `@plugwise/model.py`:
- Around line 160-163: Remove the duplicated attribute declarations so the
optional types are preserved: in the ZigBeeNode class keep a single reachable
declaration as "reachable: bool | None = None" (remove the second "reachable:
bool") and similarly for the Location class remove the duplicate
appliances/reachable lines so the first declaration (the optional/typed one)
remains; ensure no attribute is declared twice anywhere in the file so the
optional/default shapes are not overwritten.
- Around line 193-210: ApplianceType (and similarly GroupType) is too strict and
will reject existing raw values like "thermo_sensor", "heater_electric", and
"report"; update the enum definitions (ApplianceType and GroupType) to include
the missing members (e.g., THERMO_SENSOR = "thermo_sensor", HEATER_ELECTRIC =
"heater_electric", REPORT = "report") or add a generic UNKNOWN/RAW fallback
member, so model_validate() accepts existing payloads; locate and edit the
ApplianceType and the GroupType enums in the diff (ApplianceType and the
GroupType block around lines noted) and add the missing identifiers with their
exact string values.
In `@plugwise/smile.py`:
- Around line 102-108: full_xml_update currently only refreshes the typed model
via await self._request(DOMAIN_OBJECTS, new=True) leaving the internal raw XML
store _domain_objects stale; update full_xml_update so it also refreshes
_domain_objects (the parsed XML/ElementTree used by
_domain_objects.find()/findall()) after the request—either by calling the
internal request/parsing path that returns the raw DOMAIN_OBJECTS XML or by
assigning/parsing the new XML into self._domain_objects (so methods like
_get_appliances_with_offset_functionality(), set_preset(), and
set_schedule_state() see the updated XML); keep the existing notification
handling (_get_plugwise_notifications) intact.
In `@plugwise/smilecomm.py`:
- Around line 53-68: The _parse_xml function currently uses hard-coded indices
and debug print calls which will raise IndexError on smaller installs and spam
logs; remove all print() debug statements and stop accessing appliance entries
by fixed indices (appliance_in = xml_dict["domain_objects"]["appliance"][0] /
[5]); instead retrieve the appliances list from xml_dict.get("domain_objects",
{}).get("appliance", []) and either iterate over that list to validate each
entry with Appliance.model_validate(appliance_in) or validate only the first if
intended, then return PlugwiseData.model_validate(xml_dict); ensure you handle
the case where the list is empty (e.g., skip per-appliance validation or raise a
clear error) so PlugwiseData.model_validate() still runs safely.
---
Outside diff comments:
In `@plugwise/__init__.py`:
- Around line 124-130: The conditional incorrectly checks "dsmrmain is None" and
the error log references an undefined variable `result`; change the condition to
use `not dsmrmain` (since `dsmrmain` is a bool) and capture or construct the
response content to log (e.g., the variable returned by the previous
`_request()` call or the current response stored in the surrounding scope) so
that LOGGER.error in this block logs the actual response content; update the
branch around vendor_names, dsmrmain, LOGGER.error, and the ResponseError raise
to use the correct boolean check and a valid response variable.
- Around line 156-166: The call to SmileLegacyAPI is passing self.data which
doesn't match SmileLegacyAPI.__init__ (causing a TypeError); fix by removing the
trailing self.data argument from the SmileLegacyAPI(...) instantiation so the
argument list matches the constructor signature used in legacy/smile.py, or
alternatively update SmileLegacyAPI.__init__ to accept and store a data:
PlugwiseData parameter (add parameter to the constructor and assign to an
instance attribute) if the legacy class actually requires that data.
- Line 8: Remove the unused imports flagged by the linter: delete `cast` from
the `from typing import ...` line, remove `MODULES` from the `from
plugwise.constants import ...` import, and drop the `Munch` import entirely;
search for any usages of `cast`, `MODULES`, or `Munch` in the module (they
should be unused) and if none exist remove the identifiers from their import
statements so the file no longer imports these symbols.
In `@plugwise/common.py`:
- Around line 154-172: In _create_gw_entities, replace the initial dictionary
key self.gw_entities[appl.id] = {"dev_class": appl.type} with
self.gw_entities[appl.entity_id] = {"dev_class": appl.type} so the same entity
identifier (appl.entity_id) is used for creation and subsequent updates; keep
the rest of the loop as-is (the appl_key cast and assignments to
self.gw_entities[appl.entity_id][appl_key]) so no KeyError occurs when updating
entity fields.
In `@plugwise/helper.py`:
- Around line 380-389: get_appliance() returns a domain model, not an XML node,
but the code is passing that model into XML helper functions and calling
appliance.find("type"), which will crash; fix by passing the original XML node
(the existing variable entity) into the XML-specific helpers
(_appliance_measurements, _get_lock_state, _get_toggle_state, and the
actuator-type check) and only use the model from get_appliance() for places that
need model properties; specifically, change calls that currently pass appliance
into _appliance_measurements, _get_lock_state, _get_toggle_state and the
appliance.find("type").text check to use entity instead, leaving the appliance
model available for any model-based logic.
In `@plugwise/smile.py`:
- Around line 68-95: Constructor stores a mixed object in self.data and leaves a
stray print call; update __init__ so data consistently holds one model type:
either assign the domain-object container (e.g., self.data =
data.domain_objects) and keep the parameter type PlugwiseData, or change the
parameter/type to accept the inner domain object directly and assign that to
self.data (refer to __init__, self.data, and usages like self.data.location and
self.data.notification), and remove the debug print("HOI16 ...") (or replace
with a proper logger.debug) so CI is no longer affected.
In `@plugwise/smilecomm.py`:
- Around line 165-176: The branch that handles new=True currently parses and
sanitizes the response into the local variable xml but then calls
self._parse_xml on the original result string, so responses that required
escape_illegal_xml_characters() still fail; update the new=True path to pass the
already-sanitized/parsed data (use the xml variable or its decoded/text form)
into self._parse_xml (and into domain_objects) instead of using result, so
InvalidXMLError handling and subsequent assignment to self.data use the
sanitized payload.
- Around line 70-131: In _request, the retry recursive calls drop method, data
and new so retries reset to defaults; change the two recursive calls inside the
exception handler and the 504 status branch to pass the same parameters (e.g.
return await self._request(command, retry - 1, method=method, data=data,
new=new)) so _request preserves the original method, data and new flags across
retries (locate the recursive calls inside the _request function).
---
Nitpick comments:
In `@plugwise/common.py`:
- Around line 281-297: Remove or properly comment out the legacy dead-code block
that follows the return module_data no-op string literal: the triple-quoted
string starting before the legacy logic (which references appl_search, link_tag,
xml_2, self._domain_objects, get_vendor_name, get_zigbee_data, and module_data)
should either be deleted entirely or converted into line-comments prefixed with
# to avoid an executable string literal; ensure no functional code remains
inside that block and preserve any necessary notes as non-executable comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca283136-9bc2-4eec-bb91-054a5c3d0369
📒 Files selected for processing (10)
CHANGELOG.mdplugwise/__init__.pyplugwise/common.pyplugwise/constants.pyplugwise/data.pyplugwise/helper.pyplugwise/model.pyplugwise/smile.pyplugwise/smilecomm.pytests/test_init.py
| - Remove biome (as prettier was reinstated) | ||
| - Replace node-based markdownlint with pythonic library | ||
|
|
||
| - Attempt to ditch untyped Munch for the existing TypedDicts by leveraging pydantic to type xmltodict XML conversion |
Contributor
There was a problem hiding this comment.
Move this bullet back under ## Ongoing.
At its current position Line 17 renders as part of v1.11.3, not the ongoing section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 17, The changelog bullet "Attempt to ditch untyped
Munch for the existing TypedDicts by leveraging pydantic to type xmltodict XML
conversion" is currently under the v1.11.3 section and should be moved back to
the ongoing section; edit CHANGELOG.md to cut that bullet from its current
location (line containing that exact text) and paste it under the "## Ongoing"
heading so it renders as part of Ongoing instead of v1.11.3.
Comment on lines
+179
to
+180
| print(f"HOI14 {self}") | ||
| print(f"HOI14 {self.smile}") |
Contributor
There was a problem hiding this comment.
Remove debug print statements.
These print statements are debug artifacts that should not be in production code:
- Lines 179-180:
print(f"HOI14 ...") - Lines 191-192:
print(f"HOI11a/b ...") - Lines 197-199:
print(f"HOI11c ...")
🧹 Proposed fix
- print(f"HOI14 {self}")
- print(f"HOI14 {self.smile}")
model: str = "Unknown"- print(f"HOI11a {self.data.gateway}")
- print(f"HOI11b {self.data.gateway.gateway_environment}")
if (
"electricity_consumption_tariff_structure"
in self.data.gateway.gateway_environment
):
- print(
- f"HOI11c {self.data.gateway.gateway_environment.electricity_consumption_tariff_structure}"
- )Also applies to: 191-192, 197-199
🧰 Tools
🪛 GitHub Actions: Latest commit
[error] 179-179: ruff check failed: T201 print found. (Remove print)
[error] 180-180: ruff check failed: T201 print found. (Remove print)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/__init__.py` around lines 179 - 180, Remove the leftover debug print
statements that output "HOI14", "HOI11a/b" and "HOI11c" (the prints referencing
self and self.smile) from plugwise/__init__.py; locate the prints in the methods
where self and self.smile are printed and delete those print(...) calls, or
replace them with appropriate logger.debug(...) calls using the module/class
logger if you need retained diagnostics, ensuring no raw print statements remain
in production code.
Comment on lines
+208
to
+211
| # TODO | ||
| self.smile.vendor_model = await self._smile_detect_legacy( | ||
| result, dsmrmain, model | ||
| ) |
Contributor
There was a problem hiding this comment.
Critical: Undefined variables result and dsmrmain in legacy detection call.
The variables result and dsmrmain are passed to _smile_detect_legacy() but they are not defined in the scope of _smile_detect(). The dsmrmain variable exists only in connect(), and result was removed entirely during refactoring.
💡 Suggested approach
Either:
- Pass
dsmrmainas a parameter to_smile_detect()fromconnect(), or - Store it as an instance variable, or
- Refactor
_smile_detect_legacy()to work with the new model data
- async def _smile_detect(self) -> None:
+ async def _smile_detect(self, dsmrmain: bool) -> None:And in connect():
- await self._smile_detect()
+ await self._smile_detect(dsmrmain)For result, you'll need to determine what XML data _smile_detect_legacy() actually needs and pass it appropriately.
🧰 Tools
🪛 GitHub Actions: Latest commit
[error] 210-210: ruff check failed: F821 Undefined name result.
[error] 210-210: ruff check failed: F821 Undefined name dsmrmain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/__init__.py` around lines 208 - 211, The call to
_smile_detect_legacy in _smile_detect is using undefined variables result and
dsmrmain; update the code so legacy detection receives the correct data: either
add dsmrmain as a parameter to _smile_detect and pass it from connect(), or
store dsmrmain on self in connect() and read self.dsmrmain inside _smile_detect,
and replace result with the actual XML/object the legacy routine needs (or
refactor _smile_detect_legacy to accept the current model data). Locate usages
in connect(), _smile_detect(), and _smile_detect_legacy to make the
parameter/attribute consistent and ensure all callers are updated accordingly.
Comment on lines
+266
to
+277
| self._on_off_device: bool = ( | ||
| True | ||
| if "protocols" in self.data.module | ||
| and "on_off_boiler" in self.data.module.protocols | ||
| else False | ||
| ) | ||
| self._opentherm_device: bool = ( | ||
| True | ||
| if "protocols" in self.data.module | ||
| and "open_therm_boiler" in self.data.module.protocols | ||
| else False | ||
| ) |
Contributor
There was a problem hiding this comment.
Type error: self.data.module is a list, not an object with protocols attribute.
Lines 118-122 iterate over self.data.module as a list, but lines 268-276 access self.data.module.protocols as if it's a single object. This inconsistency will cause an AttributeError.
🐛 Proposed fix - iterate over modules
- self._on_off_device: bool = (
- True
- if "protocols" in self.data.module
- and "on_off_boiler" in self.data.module.protocols
- else False
- )
- self._opentherm_device: bool = (
- True
- if "protocols" in self.data.module
- and "open_therm_boiler" in self.data.module.protocols
- else False
- )
+ self._on_off_device = False
+ self._opentherm_device = False
+ for module in self.data.module:
+ if module.protocols:
+ if "on_off_boiler" in module.protocols:
+ self._on_off_device = True
+ if "open_therm_boiler" in module.protocols:
+ self._opentherm_device = True📝 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.
Suggested change
| self._on_off_device: bool = ( | |
| True | |
| if "protocols" in self.data.module | |
| and "on_off_boiler" in self.data.module.protocols | |
| else False | |
| ) | |
| self._opentherm_device: bool = ( | |
| True | |
| if "protocols" in self.data.module | |
| and "open_therm_boiler" in self.data.module.protocols | |
| else False | |
| ) | |
| self._on_off_device = False | |
| self._opentherm_device = False | |
| for module in self.data.module: | |
| if module.protocols: | |
| if "on_off_boiler" in module.protocols: | |
| self._on_off_device = True | |
| if "open_therm_boiler" in module.protocols: | |
| self._opentherm_device = True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/__init__.py` around lines 266 - 277, self.data.module is a list, but
the flags _on_off_device and _opentherm_device treat it like a single object;
update the checks for self._on_off_device and self._opentherm_device to iterate
over the modules (e.g. use a loop or any(...) across self.data.module) and check
each module's protocols for "on_off_boiler" and "open_therm_boiler"
respectively, setting the boolean to True if any module contains the protocol;
ensure you reference self.data.module and the attribute protocols on each module
item when implementing the fix.
Comment on lines
34
to
+48
| if legacy: | ||
| # Stretches | ||
| if (router := module.find("./protocols/network_router")) is not None: | ||
| module_data["zigbee_mac_address"] = router.find("mac_address").text | ||
| # Also look for the Circle+/Stealth M+ | ||
| if (coord := module.find("./protocols/network_coordinator")) is not None: | ||
| module_data["zigbee_mac_address"] = coord.find("mac_address").text | ||
| if legacy: | ||
| if module.protocols.network_router: | ||
| module_data.zigbee_mac_address = module.protocols.network_router.mac_address | ||
| if module.protocols.network_coordinator: | ||
| module_data.zigbee_mac_address = ( | ||
| module.protocols.network_coordinator.mac_address | ||
| ) | ||
| return |
Contributor
There was a problem hiding this comment.
Duplicate if legacy: blocks with mixed XML and model access.
There are two consecutive if legacy: blocks:
- Lines 34-40: Uses XML-style
.find()methods onmodule, which won't work ifmoduleis now a PydanticModulemodel - Lines 41-48: Uses model-style attribute access (
module.protocols.network_router)
The first block appears to be leftover code that should have been removed.
🐛 Proposed fix - remove the XML-based legacy block
if not module.protocols:
return
- if legacy:
- # Stretches
- if (router := module.find("./protocols/network_router")) is not None:
- module_data["zigbee_mac_address"] = router.find("mac_address").text
- # Also look for the Circle+/Stealth M+
- if (coord := module.find("./protocols/network_coordinator")) is not None:
- module_data["zigbee_mac_address"] = coord.find("mac_address").text
if legacy:
if module.protocols.network_router:
module_data.zigbee_mac_address = module.protocols.network_router.mac_address🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/common.py` around lines 34 - 48, Remove the stray XML-style legacy
block that uses module.find("./protocols/network_router") and
module.find("./protocols/network_coordinator") (the first if legacy: block that
sets module_data["zigbee_mac_address"] via router/coord variables); keep the
subsequent model-based legacy handling that accesses
module.protocols.network_router and module.protocols.network_coordinator and
assigns module_data.zigbee_mac_address, then return—i.e., delete the XML-access
block so only the Pydantic model-style legacy logic remains.
Comment on lines
+473
to
478
| for notification in self._domain_objects.notification: | ||
| try: | ||
| msg_id = notification.get("id") | ||
| msg_type = notification.find("type").text | ||
| msg = notification.find("message").text | ||
| msg_id = notification.id | ||
| msg_type = notification.type | ||
| msg = notification.message | ||
| self._notifications[msg_id] = {msg_type: msg} |
Contributor
There was a problem hiding this comment.
Normalize the singleton notification case before iterating it.
DomainObjects.notification can be a single Notification, a list, or None. Iterating it directly here breaks on the singleton case; coerce it to a one-item list first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/helper.py` around lines 473 - 478, The loop over
self._domain_objects.notification fails when DomainObjects.notification is a
singleton or None; before iterating in the block that accesses notification.id,
notification.type and notification.message and writes to self._notifications,
coerce DomainObjects.notification into an iterable: if it's None skip, if it's a
single Notification wrap it in a list, otherwise keep the list as-is; then
iterate that normalized list so the logic inside (using notification.id,
notification.type, notification.message and updating self._notifications) works
for all cases.
Comment on lines
+160
to
+163
| reachable: bool | None = None | ||
| mac_address: str | ||
| type: str | ||
| reachable: bool |
Contributor
There was a problem hiding this comment.
Remove the duplicated field declarations.
The second annotation silently overwrites the first in both classes. That makes ZigBeeNode.reachable required again on Line 163 and discards the first Location.appliances shape on Line 325.
Also applies to: 323-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/model.py` around lines 160 - 163, Remove the duplicated attribute
declarations so the optional types are preserved: in the ZigBeeNode class keep a
single reachable declaration as "reachable: bool | None = None" (remove the
second "reachable: bool") and similarly for the Location class remove the
duplicate appliances/reachable lines so the first declaration (the
optional/typed one) remains; ensure no attribute is declared twice anywhere in
the file so the optional/default shapes are not overwritten.
Comment on lines
+193
to
+210
| class ApplianceType(str, Enum): | ||
| """Define application types.""" | ||
|
|
||
| GATEWAY = "gateway" | ||
| OPENTHERMGW = "open_therm_gateway" | ||
| THERMOSTAT = "thermostat" | ||
| CHP = "central_heating_pump" | ||
| CD = "computer_desktop" | ||
| HC = "heater_central" | ||
| HT = "hometheater" | ||
| STRETCH = "stretch" | ||
| THERMO_RV = "thermostatic_radiator_valve" | ||
| VA = "valve_actuator" | ||
| VA_plug = "valve_actuator_plug" | ||
| WHV = "water_heater_vessel" | ||
| ZONETHERMOMETER = "zone_thermometer" | ||
| ZONETHERMOSTAT = "zone_thermostat" | ||
|
|
Contributor
There was a problem hiding this comment.
Broaden these enums before using them as validation gates.
ApplianceType and GroupType are now strict parse-time filters, but the rest of the repo still handles raw values that are missing here, e.g. thermo_sensor, heater_electric, and report. Any payload carrying one of those existing values will now fail model_validate() during connect.
Also applies to: 289-294
🧰 Tools
🪛 GitHub Actions: Latest commit
[error] 193-193: ruff check failed: UP042 Class ApplianceType inherits from both str and enum.Enum; inherit from enum.StrEnum.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/model.py` around lines 193 - 210, ApplianceType (and similarly
GroupType) is too strict and will reject existing raw values like
"thermo_sensor", "heater_electric", and "report"; update the enum definitions
(ApplianceType and GroupType) to include the missing members (e.g.,
THERMO_SENSOR = "thermo_sensor", HEATER_ELECTRIC = "heater_electric", REPORT =
"report") or add a generic UNKNOWN/RAW fallback member, so model_validate()
accepts existing payloads; locate and edit the ApplianceType and the GroupType
enums in the diff (ApplianceType and the GroupType block around lines noted) and
add the missing identifiers with their exact string values.
Comment on lines
102
to
+108
| async def full_xml_update(self) -> None: | ||
| """Perform a first fetch of the Plugwise server XML data.""" | ||
| self._domain_objects = await self._request(DOMAIN_OBJECTS) | ||
| self._get_plugwise_notifications() | ||
| await self._request(DOMAIN_OBJECTS, new=True) | ||
| print(f"HOI3a {self.data}") | ||
| if "notification" in self.data and self.data.notification is not None: | ||
| print(f"HOI3b {self.data.notification}") | ||
| self._get_plugwise_notifications() |
Contributor
There was a problem hiding this comment.
full_xml_update() no longer refreshes _domain_objects.
After Line 104 only the typed model is refreshed. The rest of SmileAPI still calls _domain_objects.find()/findall() in methods like _get_appliances_with_offset_functionality(), set_preset(), and set_schedule_state(), so the first update runs against stale or unset XML.
🧰 Tools
🪛 GitHub Actions: Latest commit
[error] 105-105: ruff check failed: T201 print found. (Remove print)
[error] 107-107: ruff check failed: T201 print found. (Remove print)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/smile.py` around lines 102 - 108, full_xml_update currently only
refreshes the typed model via await self._request(DOMAIN_OBJECTS, new=True)
leaving the internal raw XML store _domain_objects stale; update full_xml_update
so it also refreshes _domain_objects (the parsed XML/ElementTree used by
_domain_objects.find()/findall()) after the request—either by calling the
internal request/parsing path that returns the raw DOMAIN_OBJECTS XML or by
assigning/parsing the new XML into self._domain_objects (so methods like
_get_appliances_with_offset_functionality(), set_preset(), and
set_schedule_state() see the updated XML); keep the existing notification
handling (_get_plugwise_notifications) intact.
Comment on lines
+53
to
+68
| def _parse_xml(self, xml: str) -> dict: | ||
| """Map XML to Pydantic class.""" | ||
| element = etree.fromstring(xml) | ||
| xml_dict = xmltodict.parse(etree.tostring(element)) | ||
| print(f"HOI1 {xml_dict.keys()}") | ||
| print( | ||
| f"HOI2 {json.dumps(xmltodict.parse(xml, process_namespaces=True), indent=2)}" | ||
| ) | ||
| appliance_in = xml_dict["domain_objects"]["appliance"][0] | ||
| print(f"HOI4a1 {json.dumps(appliance_in, indent=2)}") | ||
| appliance_in = xml_dict["domain_objects"]["appliance"][5] | ||
| print(f"HOI4a1 {json.dumps(appliance_in, indent=2)}") | ||
| appliance = Appliance.model_validate(appliance_in) | ||
| print(f"HOI4a2 {appliance}") | ||
|
|
||
| return PlugwiseData.model_validate(xml_dict) |
Contributor
There was a problem hiding this comment.
Remove the hard-coded appliance probes from _parse_xml().
Lines 61-67 assume at least six appliances and will raise IndexError on smaller installs before PlugwiseData.model_validate() ever runs. The debug print()s also keep the job red.
🧰 Tools
🪛 GitHub Actions: Latest commit
[error] 57-60: ruff check failed: T201 multiple print found in this block. (Remove print)
[error] 62-67: ruff check failed: T201 multiple print found in this block. (Remove print)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/smilecomm.py` around lines 53 - 68, The _parse_xml function
currently uses hard-coded indices and debug print calls which will raise
IndexError on smaller installs and spam logs; remove all print() debug
statements and stop accessing appliance entries by fixed indices (appliance_in =
xml_dict["domain_objects"]["appliance"][0] / [5]); instead retrieve the
appliances list from xml_dict.get("domain_objects", {}).get("appliance", []) and
either iterate over that list to validate each entry with
Appliance.model_validate(appliance_in) or validate only the first if intended,
then return PlugwiseData.model_validate(xml_dict); ensure you handle the case
where the list is empty (e.g., skip per-appliance validation or raise a clear
error) so PlugwiseData.model_validate() still runs safely.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Refactor
Bug Fixes