Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR replaces untyped Munch/XML parsing with Pydantic-validated typed models (PlugwiseData) and updates core components (init, comms, helpers, common, smile API, util, tests) to consume the new model structures instead of raw XML/dict access. 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: _request(DOMAIN_OBJECTS, new=True)
Request->>Request: HTTP fetch raw XML
Request->>Parser: pass raw response text
Parser->>Parser: parse XML (defusedxml) -> dict (xmltodict)
Parser->>Validator: validate dict with PlugwiseData
Validator->>Validator: build runtime indexes (model_post_init)
Validator->>Storage: return validated DomainObjects/PlugwiseData
Storage->>Storage: assign self.data
Storage-->>Client: return original response string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
🚥 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
+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
150
to
170
| locator = MODULE_LOCATOR | ||
| tag = "electricity" | ||
| module_data = self._get_module_data(self._home_location, locator, key=tag) | ||
| # No module-data present means the device has been removed | ||
| if not module_data["contents"]: # pragma: no cover | ||
| if not module_data["content"]: # pragma: no cover | ||
| return | ||
|
|
||
| appl.available = None | ||
| appl.entity_id = self._home_loc_id | ||
| if not self.smile.anna_p1: | ||
| appl.entity_id = self._gateway_id | ||
| appl.firmware = module_data["firmware_version"] | ||
| appl.hardware = module_data["hardware_version"] | ||
| appl.firmware = module_data.firmware_version | ||
| appl.hardware = module_data.hardware_version | ||
| appl.location = self._home_loc_id | ||
| appl.mac = None | ||
| appl.model = module_data["vendor_model"] | ||
| appl.model = module_data.vendor_model | ||
| appl.model_id = None # don't use model_id for SmartMeter | ||
| appl.name = "P1" | ||
| appl.pwclass = "smartmeter" | ||
| appl.vendor_name = module_data["vendor_name"] | ||
| appl.vendor_name = module_data.vendor_name | ||
| appl.zigbee_mac = None |
Contributor
There was a problem hiding this comment.
This P1 path still uses the pre-model API.
Line 154 still treats module_data as a dict, while Lines 157-170 populate an undefined appl. The first P1/Anna P1 setup will hit TypeError or NameError before the smartmeter entity is created.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/helper.py` around lines 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.
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.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
plugwise/smilecomm.py (1)
8-24:⚠️ Potential issue | 🔴 CriticalPipeline failures: remove unused imports
jsonandAppliance.Ruff flags
json(line 8) andAppliance(line 24) as unused; both were added for the now-removed debug scaffolding. These block CI.🧹 Fix
-import json # Debugging - from plugwise.constants import LOGGER @@ -import xmltodict - -from .model import Appliance, PlugwiseData +import xmltodict + +from .model import PlugwiseData🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/smilecomm.py` around lines 8 - 24, Remove the unused imports causing the Ruff failure: delete the top-level import of the json module and remove the Appliance symbol from the from .model import list in smilecomm.py; keep the remaining imports (e.g., PlugwiseData) intact so other code still references PlugwiseData and no other import lines are altered.plugwise/__init__.py (2)
8-41:⚠️ Potential issue | 🔴 CriticalPipeline: remove unused imports blocking CI.
Ruff F401 flags
typing.cast(line 8),plugwise.constants.MODULES(line 17), andmunch.Munch(line 41) — all still imported but no longer referenced after the refactor.🧹 Fix
-from typing import cast @@ - DOMAIN_OBJECTS, - LOGGER, - MODULES, - NONE, + DOMAIN_OBJECTS, + LOGGER, + NONE, @@ -from munch import Munch from packaging.version import Version, parse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/__init__.py` around lines 8 - 41, Remove the now-unused imports causing Ruff F401: drop typing.cast, plugwise.constants.MODULES, and munch.Munch from plugwise/__init__.py; update the import block that currently references cast (line 8), MODULES (in the constants import list), and Munch (imported at bottom) so only the actually used names (e.g., DEFAULT_TIMEOUT, LOGGER, SmileAPI, SmileComm, etc.) remain, ensuring no other references to cast, MODULES, or Munch exist in the module.
110-140:⚠️ Potential issue | 🔴 CriticalMultiple correctness bugs in the new
connect()pre-detection block.Three problems in the same flow, any one of which breaks first-connect:
- Line 124 –
dsmrmainwas redefined asbool = Falseon line 115, sodsmrmain is Noneis alwaysFalse; the only way to hit the error branch is now "no Plugwise in vendor names AND dsmrmain is None" — theandshould beor, and theNoneshould benot dsmrmain.- Line 128 –
resultis no longer defined in this scope after the XML pathway was removed (Ruff F821). The old log message referenced the raw XML text; with the model flow there's nothing useful to log here — drop the argument or log the vendor names.self.data.gatewayshape –DomainObjects.gatewayis typedGateway | list[Gateway] | None, but downstream code (line 182 onward) assumes a singleGateway. If a payload ever surfaces a list, the.vendor_model/.firmware_version/.gateway_environmentaccesses blow up. Normalize to a list (or to a single item) once at the top of_smile_detect().🩹 Proposed fix for (1) and (2)
- 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 ResponseErrorplugwise/helper.py (3)
18-52:⚠️ Potential issue | 🟡 MinorPipeline: remove the five unused imports blocking Ruff.
ATTR_NAME,ENERGY_WATT_HOUR,SensorType,OffsetFunctionality,skip_obsolete_measurementsare all unused after the refactor (all flagged by Ruff F401). Once_appliance_measurementsis restored with real logic, some may come back — but keeping them now keeps CI red for no benefit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/helper.py` around lines 18 - 52, Remove the five unused imports flagged by Ruff: drop ATTR_NAME and ENERGY_WATT_HOUR from the first import block, remove SensorType and OffsetFunctionality from the plugwise.model import list, and remove skip_obsolete_measurements from the plugwise.util import list; update the import statements so only used symbols (e.g., DATA, DEVICE_MEASUREMENTS, ActuatorData, etc.) remain to clear the F401 warnings while leaving _appliance_measurements and related logic untouched.
848-855:⚠️ Potential issue | 🟠 Major
_heating_valvesstill walksself._domain_objectswhich is no longer refreshed.
full_xml_updateinplugwise/smile.pynow only populatesself.data;self._domain_objectsis either stale or unset. This function therefore either returnsFalsespuriously or raises on the first Adam with external heat. Same concern applies throughout this file to_preset,_presets,_rule_ids_by_name,_rule_ids_by_tag,_schedules,_thermostat_uri,_get_zone_data,_collect_group_sensors,_get_actuator_functionalities, anddetermine_contexts/set_preset/set_schedule_state/set_number/set_switch_stateinplugwise/smile.py.This is the systemic follow-through the previous reviewer flagged on
full_xml_update: until_domain_objectsis re-populated (or every XML traversal is migrated to the model), most non-trivial flows silently operate on stale data. Please track the remaining XML-path migrations in a single issue so nothing is missed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/helper.py` around lines 848 - 855, The _heating_valves function (and other helpers like _preset, _presets, _rule_ids_by_name, _rule_ids_by_tag, _schedules, _thermostat_uri, _get_zone_data, _collect_group_sensors, _get_actuator_functionalities, determine_contexts, set_preset, set_schedule_state, set_number, set_switch_state) currently walks self._domain_objects which is no longer refreshed by full_xml_update; update each of these functions to read from the new model stored in self.data (or ensure full_xml_update repopulates self._domain_objects) so XML traversals use the fresh parsed model instead of stale state, and list the remaining functions still using _domain_objects in a single tracking issue so all usages are migrated consistently.
250-272:⚠️ Potential issue | 🔴 Critical
_appl_gateway_infoincorrectly accesses Appliance model attributes that don't exist.
- Line 257:
appliance.protocols.zig_bee_coordinator—Appliancemodels have noprotocolsfield (that's onModule). This causes anAttributeErroron any Adam gateway.- Line 258: Assigns to
appliance.zigbee_mac, but the correct field isappliance.zigbee_mac_address.- Line 265: Passes
appliance(Pydantic model) to_get_appl_actuator_modes()which expectsetree.Element.- Line 268: Calls
appliance.find(locator)on a Pydantic model, which has no.find()method—this entire branch is dead code.Use
self.data.get_module(appliance.id)to access the Module and readzigbee_mac_addressfrom there, and usesearch_actuator_functionalities(appliance, "gateway_mode_control_functionality")to access actuator functionalities instead of XML XPath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/helper.py` around lines 250 - 272, _appl_gateway_info currently treats the Pydantic Appliance like an XML/Module object; fix it by retrieving the Module via self.data.get_module(appliance.id) and reading module.zigbee_mac_address (assign to appliance.zigbee_mac_address) instead of appliance.protocols.zig_bee_coordinator/zigbee_mac, pass the Module's XML/Element (e.g. module.etree or module.element) into _get_appl_actuator_modes (since _get_appl_actuator_modes expects an etree.Element) instead of the Appliance, and replace the appliance.find(locator) XPath logic with search_actuator_functionalities(appliance, "gateway_mode_control_functionality") (or the module/element variant) to populate self._gw_allowed_modes = ["away","full","vacation"] when a gateway_mode_control_functionality exists; update references to these symbols: _appl_gateway_info, self.data.get_module, Module (module), appliance.zigbee_mac_address, _get_appl_actuator_modes, and search_actuator_functionalities.
♻️ Duplicate comments (3)
plugwise/model.py (2)
186-195:⚠️ Potential issue | 🟠 MajorDuplicate attribute declarations still in tree (previous fix did not land).
ZigBeeNode.reachableis declared twice (line 186bool | None = None, line 189bool— the second overwrites and makes it required).Location.appliancesis declared twice (line 358list[WithID]required; line 360dict[...] | None = Nonewhich overwrites and changes both type and optionality).Either redeclaration is a footgun; the second overrides the first in Pydantic v2 without warning. Remove the stale declaration in each pair.
🩹 Fix
class ZigBeeNode(WithID): """ZigBee node definition.""" reachable: bool | None = None mac_address: str type: str - reachable: bool power_source: str | None = None @@ preset: str | None = None - appliances: list[WithID] logs: dict[str, BaseLog | list[BaseLog]] | list[BaseLog] | None appliances: dict[str, WithID | list[WithID]] | None = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/model.py` around lines 186 - 195, ZigBeeNode.reachable and Location.appliances are declared twice causing the latter declarations to overwrite the intended types/optionality; remove the duplicate (stale) declarations so only the correct definitions remain: keep the optional reachable: bool | None = None on the ZigBeeNode model and remove the second reachable: bool, and keep the intended appliances type/optionality (either list[WithID] or dict[...] | None = None per the original design) and remove the conflicting duplicate declaration so Pydantic v2 sees the correct single attribute definition for Location.appliances.
219-236:⚠️ Potential issue | 🔴 Critical
ApplianceTypeenum still too narrow — will failmodel_validateon common payloads.(Duplicate of prior review.) The codebase elsewhere still references
thermo_sensor,heater_electric,report, etc., and fixtures undertests/include those values. WithAppliance.type: ApplianceTypestrict validation, any such payload raisesValidationErrorduring the very firstPlugwiseData.model_validate()in_request_validate. Please either widen the enum to cover all real-world values or relax tostrwith a light enum-style sentinel layer.Also: Ruff UP042 — inherit from
enum.StrEnumrather than(str, Enum)(applies toApplianceType,GroupType,SwitchDeviceType,SwitchFunctionType,SwitchActuatorType).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/model.py` around lines 219 - 236, ApplianceType is too narrow and will make PlugwiseData.model_validate() fail for real payloads (Appliance.type), so either expand ApplianceType to include all observed values (e.g., thermo_sensor, heater_electric, report, etc.) used across tests and code, or change Appliance.type to plain str (and keep an optional light sentinel/validation layer) so unknown values pass model_validate; additionally, replace all Enum definitions currently inheriting from (str, Enum) — specifically ApplianceType, GroupType, SwitchDeviceType, SwitchFunctionType, and SwitchActuatorType — to inherit from enum.StrEnum to satisfy Ruff UP042 and preserve string behavior.plugwise/common.py (1)
35-49:⚠️ Potential issue | 🔴 CriticalDuplicate legacy block still present — regression of previously addressed feedback.
Lines 35–41 still contain the old XML-based
module.find("./protocols/network_router")/...network_coordinatorblock even though the model-based equivalent exists on lines 42–49 and the previous review noted this was fixed.moduleis now aModulePydantic model and has no.find()method, so the first legacy call will raiseAttributeErrorbefore reaching the model branch.🩹 Fix
- 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 35 - 49, Remove the obsolete XML-based legacy block that calls module.find("./protocols/network_router") and module.find("./protocols/network_coordinator") (which will raise AttributeError because module is now a Pydantic Module), and rely only on the model-based branch that checks module.protocols.network_router and module.protocols.network_coordinator to set module_data.zigbee_mac_address; keep the existing assignments using module.protocols.network_router.mac_address and module.protocols.network_coordinator.mac_address and the final return, deleting the duplicate legacy if-block entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugwise/__init__.py`:
- Line 172: The connect() function currently annotated as returning Version
actually returns self.smile.firmware_version which is a str | None
(GatewayData.firmware_version); fix by either changing connect()'s return
annotation to -> str | None and update callers accordingly, or convert/parse
self.smile.firmware_version to a Version before returning (e.g., handle None and
construct a Version object or raise), and also reconcile
GatewayData.model_post_init where self.smile.version (runtime Version) is stored
into a str-annotated field—make the field/assignment types consistent (either
annotate firmware_version/version as Version or ensure stored value is a str).
- Around line 115-122: The loop over self.data.module must guard against
module.protocols and module.vendor_name/module.vendor_model being None: only
append module.vendor_name to vendor_names and module.vendor_model to
vendor_models when they are not None, and determine dsmrmain only when
module.protocols is present and supports membership; implement this by checking
if module.protocols is not None (or isinstance(module.protocols, dict) and then
"dsmrmain" in module.protocols, or if it's a list check elements appropriately),
or by wrapping the "dsmrmain" membership test in a try/except TypeError to avoid
calling "in" on None, and update the variables vendor_names/vendor_models and
dsmrmain accordingly.
In `@plugwise/common.py`:
- Around line 241-244: The inner walrus is binding the result of the comparison
instead of the call due to operator precedence causing func_type to become a
bool and later .lock to fail; change the if so the walrus assigns the result of
actuator.get(func_type) (not the boolean) before the is not None check (e.g. use
if (func_type := actuator.get(func_type)) is not None:), and avoid shadowing the
parameter name by renaming the outer walrus target (the result of getattr) to a
different local variable (e.g. act or act_obj) so _get_lock_state uses the local
act/act_obj, func_type, data["switches"]["lock"], and self._count correctly.
- Around line 21-22: The import line imports unused symbols DomainObjects and
return_valid causing a linter failure; remove these unused imports from the from
plugwise.model / from plugwise.util import statement (keep Appliance,
check_heater_central, check_model) so the import becomes only the used names, or
if they are intended to be used add the missing usage. Specifically edit the
import that currently references DomainObjects and return_valid to eliminate
those two symbols.
- Around line 246-281: The _get_module_data function currently has the wrong
signature and treats self.data.appliance as a single Appliance; change its
signature to accept an appliance parameter (e.g., def _get_module_data(self,
appliance: Appliance, key: str | None = None, legacy: bool = False) ->
ModuleData) and update the body to operate on that appliance (check
appliance.services, iterate appliance.services.iter_services() or the actual
service iterable, call self.data.get_module(service.id) as before, populate
ModuleData and call get_zigbee_data(module, module_data, legacy)); update
callers that pass an appliance to pass it into the new first argument;
alternatively if you prefer the other approach, iterate self.data.appliance (the
list) and find the matching Appliance by service/module when key is provided;
finally remove the unreachable commented-out legacy block after the function.
In `@plugwise/helper.py`:
- Around line 413-465: _appliance_measurements currently does not populate
measurements because the core logic is commented out and debug prints remain;
restore the measurement-collection flow in _appliance_measurements by
re-enabling and adapting the previous match/assign code that writes into
data["sensors"] and data["binary_sensors"], ensuring you use the provided attrs
(from the measurements dict) when mapping names and units, remove the debug
print calls (HOI10) to satisfy Ruff T201, avoid the unused-variable warning by
actually referencing attrs (Ruff B007), and iterate appliance.logs defensively
(e.g., check appliance.logs is truthy and use .get("point_log", []) /
.get("interval_log", []) or attribute-safe access) so accessing
point_log/interval_log cannot raise if logs is None or non-subscriptable;
finally keep the call to self._count = count_data_items(self._count, data) after
repopulating data.
- Line 80: The field self._gateway_id is annotated as str but initialized to
None, causing a type mismatch; change its annotation to allow None (e.g. str |
None or Optional[str]) and update any accessors and the gateway_id property
signature to reflect optionality (adjust return type from -> str to -> str |
None or Optional[str]) and handle None safely where accessed; apply the same
pattern to other members/methods mentioned in the review (for example update the
return annotation of _get_actuator_mode if it can return None and adjust callers
accordingly).
- Around line 60-66: The function search_actuator_functionalities has a wrong
return annotation and callers still expect XML methods; change its return type
from Appliance | None to BaseFunctionality | list[BaseFunctionality] | None
(matching Appliance.actuator_functionalities values) and update its docstring
accordingly, then fix callers such as _get_actuator_mode and
_get_appl_actuator_modes to treat the result as a Pydantic model (or list): do
not call .find(); instead access model attributes (e.g., .mode, .allowed_modes)
and if a list is returned iterate or choose the appropriate item before reading
those attributes.
- Around line 198-201: Rename the ambiguous generator variable in the assignment
to self._home_location so the comprehension uses a clear name (e.g., replace "(l
for l in self.data.location if l.id == loc.loc_id)" with "(loc_item for loc_item
in self.data.location if loc_item.id == loc.loc_id)") to address Ruff E741, and
change the equality check "selected == None" to the identity check "selected is
None" (wherever the variable selected is used, e.g., around the logic that
checks selection) to fix Ruff E711.
In `@plugwise/model.py`:
- Around line 342-361: The logs field on both Group and Location is currently
typed to allow None but has no default, making it required by Pydantic; update
the class attributes for logs in the Group and Location classes (the logs
declarations) to set a default of None (e.g., add " = None") so Pydantic treats
missing logs in incoming payloads as optional and avoids ValidationError during
PlugwiseData.model_validate().
- Around line 444-462: GatewayData.version is annotated as str but
model_post_init replaces it with a packaging.version.Version instance; change
the field annotation to Version (and default to Version("0.0.0") or None) or
else stop overwriting it and store the parsed Version in a new attribute (e.g.,
parsed_version) so runtime type matches annotation; update the model_post_init
method accordingly (refer to GatewayData.version and
GatewayData.model_post_init). Also reconcile the connect() return type in
plugwise.__init__ (annotated -> Version) with what it actually returns
(self.smile.firmware_version) by either returning a Version instance or changing
the annotation to str | None so signatures and runtime values match.
In `@plugwise/smile.py`:
- Around line 103-109: In full_xml_update(), remove the incorrect membership
test ("notification" in self.data) since self.data is a Pydantic BaseModel;
instead rely solely on the None check (if self.data.notification is not None) to
call _get_plugwise_notifications(); while modifying
_get_plugwise_notifications(), ensure it accepts DomainObjects.notification
which can be Notification | list[Notification] | None and normalize a singleton
Notification into an iterable (e.g., wrap non-list values into a list) before
iterating so the code handles both single and list notifications (see
DomainObjects.notification and helper.py for prior notes).
In `@plugwise/smilecomm.py`:
- Around line 172-177: Assign and treat self.data as DomainObjects: keep the
assignment in SmileComm (self.data = root.domain_objects) but make the contract
explicit by updating the Smile class annotation from PlugwiseData to
DomainObjects and add a declaration on the base SmileComm (e.g., declare
self.data: DomainObjects = None or a class attribute) so the attribute is
defined on the base class and type-consistent for all subclasses; update any
imports/typing to reference DomainObjects where needed.
- Around line 53-68: The _parse_xml function is annotated to return dict but
actually returns a PlugwiseData Pydantic model and still contains a leftover
commented debug block; update the signature of _parse_xml to return PlugwiseData
instead of dict and remove the entire commented-out debug block (the
print/xml-to-json/appliance examples) so the method contains only the parsing
steps (etree.fromstring, xmltodict.parse, and return
PlugwiseData.model_validate(xml_dict)); keep references to PlugwiseData and the
existing xml parsing calls unchanged.
In `@plugwise/util.py`:
- Around line 79-87: The type annotation for the check_heater_central function
is incorrect: update the parameter type from PlugwiseData to DomainObjects (and
import DomainObjects) because callers pass root.domain_objects; adjust the
function signature for check_heater_central(data: DomainObjects) and update the
import list to include DomainObjects so iteration over data.appliance matches
the actual object shape used by smilecomm.py and common.py.
In `@tests/test_adam.py`:
- Around line 19-30: The commented-out test_connect_adam_bad_thermostat should
be resolved: either restore and re-enable the test (uncomment, add
pytest.mark.asyncio, ensure test uses smile_setup "adam_bad_thermostat" and
asserts the "Thermostat data in Zone" warning emitted by _regulation_control) if
that warning path is still reachable, or remove the commented block entirely and
add a PR note explaining why the scenario is no longer reachable after the
refactor; reference test_connect_adam_bad_thermostat, the asserted string
"Thermostat data in Zone", and the helper function _regulation_control when
making the change.
---
Outside diff comments:
In `@plugwise/__init__.py`:
- Around line 8-41: Remove the now-unused imports causing Ruff F401: drop
typing.cast, plugwise.constants.MODULES, and munch.Munch from
plugwise/__init__.py; update the import block that currently references cast
(line 8), MODULES (in the constants import list), and Munch (imported at bottom)
so only the actually used names (e.g., DEFAULT_TIMEOUT, LOGGER, SmileAPI,
SmileComm, etc.) remain, ensuring no other references to cast, MODULES, or Munch
exist in the module.
In `@plugwise/helper.py`:
- Around line 18-52: Remove the five unused imports flagged by Ruff: drop
ATTR_NAME and ENERGY_WATT_HOUR from the first import block, remove SensorType
and OffsetFunctionality from the plugwise.model import list, and remove
skip_obsolete_measurements from the plugwise.util import list; update the import
statements so only used symbols (e.g., DATA, DEVICE_MEASUREMENTS, ActuatorData,
etc.) remain to clear the F401 warnings while leaving _appliance_measurements
and related logic untouched.
- Around line 848-855: The _heating_valves function (and other helpers like
_preset, _presets, _rule_ids_by_name, _rule_ids_by_tag, _schedules,
_thermostat_uri, _get_zone_data, _collect_group_sensors,
_get_actuator_functionalities, determine_contexts, set_preset,
set_schedule_state, set_number, set_switch_state) currently walks
self._domain_objects which is no longer refreshed by full_xml_update; update
each of these functions to read from the new model stored in self.data (or
ensure full_xml_update repopulates self._domain_objects) so XML traversals use
the fresh parsed model instead of stale state, and list the remaining functions
still using _domain_objects in a single tracking issue so all usages are
migrated consistently.
- Around line 250-272: _appl_gateway_info currently treats the Pydantic
Appliance like an XML/Module object; fix it by retrieving the Module via
self.data.get_module(appliance.id) and reading module.zigbee_mac_address (assign
to appliance.zigbee_mac_address) instead of
appliance.protocols.zig_bee_coordinator/zigbee_mac, pass the Module's
XML/Element (e.g. module.etree or module.element) into _get_appl_actuator_modes
(since _get_appl_actuator_modes expects an etree.Element) instead of the
Appliance, and replace the appliance.find(locator) XPath logic with
search_actuator_functionalities(appliance, "gateway_mode_control_functionality")
(or the module/element variant) to populate self._gw_allowed_modes =
["away","full","vacation"] when a gateway_mode_control_functionality exists;
update references to these symbols: _appl_gateway_info, self.data.get_module,
Module (module), appliance.zigbee_mac_address, _get_appl_actuator_modes, and
search_actuator_functionalities.
In `@plugwise/smilecomm.py`:
- Around line 8-24: Remove the unused imports causing the Ruff failure: delete
the top-level import of the json module and remove the Appliance symbol from the
from .model import list in smilecomm.py; keep the remaining imports (e.g.,
PlugwiseData) intact so other code still references PlugwiseData and no other
import lines are altered.
---
Duplicate comments:
In `@plugwise/common.py`:
- Around line 35-49: Remove the obsolete XML-based legacy block that calls
module.find("./protocols/network_router") and
module.find("./protocols/network_coordinator") (which will raise AttributeError
because module is now a Pydantic Module), and rely only on the model-based
branch that checks module.protocols.network_router and
module.protocols.network_coordinator to set module_data.zigbee_mac_address; keep
the existing assignments using module.protocols.network_router.mac_address and
module.protocols.network_coordinator.mac_address and the final return, deleting
the duplicate legacy if-block entirely.
In `@plugwise/model.py`:
- Around line 186-195: ZigBeeNode.reachable and Location.appliances are declared
twice causing the latter declarations to overwrite the intended
types/optionality; remove the duplicate (stale) declarations so only the correct
definitions remain: keep the optional reachable: bool | None = None on the
ZigBeeNode model and remove the second reachable: bool, and keep the intended
appliances type/optionality (either list[WithID] or dict[...] | None = None per
the original design) and remove the conflicting duplicate declaration so
Pydantic v2 sees the correct single attribute definition for
Location.appliances.
- Around line 219-236: ApplianceType is too narrow and will make
PlugwiseData.model_validate() fail for real payloads (Appliance.type), so either
expand ApplianceType to include all observed values (e.g., thermo_sensor,
heater_electric, report, etc.) used across tests and code, or change
Appliance.type to plain str (and keep an optional light sentinel/validation
layer) so unknown values pass model_validate; additionally, replace all Enum
definitions currently inheriting from (str, Enum) — specifically ApplianceType,
GroupType, SwitchDeviceType, SwitchFunctionType, and SwitchActuatorType — to
inherit from enum.StrEnum to satisfy Ruff UP042 and preserve string behavior.
🪄 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: de5d955d-b958-43ca-a236-3e213b9f86fa
📒 Files selected for processing (10)
plugwise/__init__.pyplugwise/common.pyplugwise/helper.pyplugwise/model.pyplugwise/smile.pyplugwise/smilecomm.pyplugwise/util.pyrequirements_test.txttests/test_adam.pytests/test_init.py
✅ Files skipped from review due to trivial changes (2)
- requirements_test.txt
- tests/test_init.py
Comment on lines
+115
to
+122
| dsmrmain: bool = False | ||
| vendor_names: list = [] | ||
| vendor_models: list = [] | ||
| for module in self.data.module: | ||
| vendor_names.append(module.vendor_name) | ||
| vendor_models.append(module.vendor_model) | ||
| if "dsmrmain" in module.protocols: | ||
| dsmrmain = True |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Module.protocols typing and any fixtures with protocols=None
ast-grep --pattern 'class Module($_):
$$$'
rg -nP '"dsmrmain"' --type=pyRepository: plugwise/python-plugwise
Length of output: 1189
🏁 Script executed:
#!/bin/bash
# Read the full context around the problematic code
sed -n '105,130p' plugwise/__init__.py | cat -n
# Search for test fixtures or actual data with None values
grep -r "protocols.*None" --include="*.py" | head -20
grep -r "vendor_name.*None" --include="*.py" | head -20Repository: plugwise/python-plugwise
Length of output: 2469
🏁 Script executed:
#!/bin/bash
# Find where Module objects are created/populated with data
grep -n "Module(" plugwise/*.py | head -20
# Check the XML parsing in common.py to see how protocols are set
sed -n '1,100p' plugwise/common.py | grep -A 5 -B 5 "protocols"
# Search for test data or fixtures with Module objects
find . -path ./node_modules -prune -o -name "*.json" -o -name "*.xml" | xargs grep -l "protocols" 2>/dev/null | head -5Repository: plugwise/python-plugwise
Length of output: 1573
🏁 Script executed:
#!/bin/bash
# Check test data structure to see if protocols/vendor_name are optional
head -200 ./userdata/adam_jip/core.domain_objects.xml | grep -A 5 -B 5 "module" | head -50
# Also check if there's any module without protocols in test data
grep -c "<protocols" ./userdata/adam_jip/core.domain_objects.xml || echo "No protocols found"
</parameter>Repository: plugwise/python-plugwise
Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Count total modules and modules with protocols to see if any are missing
echo "Total modules in adam_jip:"
grep -c "<module " ./userdata/adam_jip/core.domain_objects.xml
echo "Modules with protocols:"
grep -c "<protocols" ./userdata/adam_jip/core.domain_objects.xml
# Check another test file
echo ""
echo "Total modules in adam_bad_thermostat:"
grep -c "<module " ./userdata/adam_bad_thermostat/core.domain_objects.xml
echo "Modules with protocols in adam_bad_thermostat:"
grep -c "<protocols" ./userdata/adam_bad_thermostat/core.domain_objects.xml
# Look at the actual code that parses modules to see what happens with missing protocols
sed -n '1,300p' plugwise/common.py | grep -A 10 "def.*module" | head -30Repository: plugwise/python-plugwise
Length of output: 1110
Guard against module.protocols and vendor_name/vendor_model being None when iterating.
Module.protocols is typed dict[str, Protocols] | list[Protocols] | None and vendor_name/vendor_model are str | None. The loop unconditionally calls "dsmrmain" in module.protocols (raises TypeError when None) and appends None values into vendor_names/vendor_models, making downstream checks fragile. The codebase elsewhere explicitly sets these fields to None and guards against None protocols in helper functions, confirming this is a real issue.
🩹 Proposed fix
- for module in self.data.module:
- vendor_names.append(module.vendor_name)
- vendor_models.append(module.vendor_model)
- if "dsmrmain" in module.protocols:
- dsmrmain = True
+ for module in self.data.module:
+ if module.vendor_name:
+ vendor_names.append(module.vendor_name)
+ if module.vendor_model:
+ vendor_models.append(module.vendor_model)
+ if module.protocols and "dsmrmain" in module.protocols:
+ dsmrmain = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/__init__.py` around lines 115 - 122, The loop over self.data.module
must guard against module.protocols and module.vendor_name/module.vendor_model
being None: only append module.vendor_name to vendor_names and
module.vendor_model to vendor_models when they are not None, and determine
dsmrmain only when module.protocols is present and supports membership;
implement this by checking if module.protocols is not None (or
isinstance(module.protocols, dict) and then "dsmrmain" in module.protocols, or
if it's a list check elements appropriately), or by wrapping the "dsmrmain"
membership test in a try/except TypeError to avoid calling "in" on None, and
update the variables vendor_names/vendor_models and dsmrmain accordingly.
| await self._smile_api.full_xml_update() | ||
|
|
||
| return cast(Version, self.smile.version) | ||
| return self.smile.firmware_version |
Contributor
There was a problem hiding this comment.
Return-type mismatch: connect() is annotated -> Version but returns str | None.
GatewayData.firmware_version is str | None, so self.smile.firmware_version cannot satisfy the declared Version return type. Either change the signature to -> str | None (and update callers) or parse before returning:
- return self.smile.firmware_version
+ return parse(self.smile.firmware_version) if self.smile.firmware_version else Version("0.0.0")Note also that self.smile.version in GatewayData.model_post_init stores a Version object into a str-annotated field (see plugwise/model.py line 457–462) — same annotation-vs-runtime issue at the other end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/__init__.py` at line 172, The connect() function currently annotated
as returning Version actually returns self.smile.firmware_version which is a str
| None (GatewayData.firmware_version); fix by either changing connect()'s return
annotation to -> str | None and update callers accordingly, or convert/parse
self.smile.firmware_version to a Version before returning (e.g., handle None and
construct a Version object or raise), and also reconcile
GatewayData.model_post_init where self.smile.version (runtime Version) is stored
into a str-annotated field—make the field/assignment types consistent (either
annotate firmware_version/version as Version or ensure stored value is a str).
Comment on lines
+21
to
+22
| from plugwise.model import Appliance, DomainObjects | ||
| from plugwise.util import check_heater_central, check_model, return_valid |
Contributor
There was a problem hiding this comment.
Pipeline: unused imports DomainObjects and return_valid (Ruff F401).
Both block CI.
-from plugwise.model import Appliance, DomainObjects
-from plugwise.util import check_heater_central, check_model, return_valid
+from plugwise.model import Appliance
+from plugwise.util import check_heater_central, check_model📝 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
| from plugwise.model import Appliance, DomainObjects | |
| from plugwise.util import check_heater_central, check_model, return_valid | |
| from plugwise.model import Appliance | |
| from plugwise.util import check_heater_central, check_model |
🧰 Tools
🪛 GitHub Actions: Latest commit
[error] 21-21: Ruff F401: plugwise.model.DomainObjects imported but unused.
[error] 22-22: Ruff F401: plugwise.util.return_valid imported but unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/common.py` around lines 21 - 22, The import line imports unused
symbols DomainObjects and return_valid causing a linter failure; remove these
unused imports from the from plugwise.model / from plugwise.util import
statement (keep Appliance, check_heater_central, check_model) so the import
becomes only the used names, or if they are intended to be used add the missing
usage. Specifically edit the import that currently references DomainObjects and
return_valid to eliminate those two symbols.
Comment on lines
+241
to
244
| if (actuator := getattr(appl, actuator, None)) is not None: | ||
| if func_type := actuator.get(func_type) is not None: | ||
| data["switches"]["lock"] = func_type.lock | ||
| self._count += 1 |
Contributor
There was a problem hiding this comment.
Walrus-operator precedence bug makes _get_lock_state completely broken.
if (actuator := getattr(appl, actuator, None)) is not None:
if func_type := actuator.get(func_type) is not None:
data["switches"]["lock"] = func_type.lock:= has lower precedence than is not None, so the inner line parses as:
if func_type := (actuator.get(func_type) is not None):…which assigns True/False to func_type and then calls .lock on a bool when truthy → AttributeError. (The outer walrus also shadows the parameter actuator, which is brittle but not functionally wrong.)
🩹 Proposed fix
- if (actuator := getattr(appl, actuator, None)) is not None:
- if func_type := actuator.get(func_type) is not None:
- data["switches"]["lock"] = func_type.lock
- self._count += 1
+ actuator_obj = getattr(appl, actuator, None)
+ if actuator_obj is None:
+ return
+ fn = actuator_obj.get(func_type)
+ if fn is not None:
+ data["switches"]["lock"] = fn.lock
+ self._count += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/common.py` around lines 241 - 244, The inner walrus is binding the
result of the comparison instead of the call due to operator precedence causing
func_type to become a bool and later .lock to fail; change the if so the walrus
assigns the result of actuator.get(func_type) (not the boolean) before the is
not None check (e.g. use if (func_type := actuator.get(func_type)) is not
None:), and avoid shadowing the parameter name by renaming the outer walrus
target (the result of getattr) to a different local variable (e.g. act or
act_obj) so _get_lock_state uses the local act/act_obj, func_type,
data["switches"]["lock"], and self._count correctly.
Comment on lines
246
to
+281
| def _get_module_data( | ||
| self, | ||
| xml_1: etree.Element, | ||
| locator: str, | ||
| key: str | None = None, | ||
| xml_2: etree.Element | None = None, | ||
| legacy: bool = False, | ||
| ) -> ModuleData: | ||
| """Helper-function for _energy_device_info_finder() and _appliance_info_finder(). | ||
|
|
||
| Collect requested info from MODULES. | ||
| """ | ||
| module_data: ModuleData = { | ||
| "contents": False, | ||
| "firmware_version": None, | ||
| "hardware_version": None, | ||
| "reachable": None, | ||
| "vendor_name": None, | ||
| "vendor_model": None, | ||
| "zigbee_mac_address": None, | ||
| } | ||
|
|
||
| for appl_search in xml_1.findall(locator): | ||
| link_tag = appl_search.tag | ||
| if key is not None and key not in link_tag: | ||
| module_data = ModuleData() | ||
| if "services" not in self.data.appliance or not self.data.appliance.services: | ||
| return module_data | ||
|
|
||
| for service_type, services in self.data.appliance.services.iter_services(): | ||
| if key and key not in service_type: | ||
| continue | ||
|
|
||
| # NOW correctly nested | ||
| for service in services: | ||
| module = self.data.get_module(service.id) | ||
| if not module: | ||
| continue | ||
|
|
||
| module_data = ModuleData( | ||
| content=True, | ||
| firmware_version=module.firmware_version, | ||
| hardware_version=module.hardware_version, | ||
| reachable=module.reachable, | ||
| vendor_name=module.vendor_name, | ||
| vendor_model=module.vendor_model, | ||
| zigbee_mac_address=module.zigbee_mac_address, | ||
| ) | ||
| get_zigbee_data(module, module_data, legacy) | ||
|
|
||
| return module_data | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm services typing and iter_services location
ast-grep --pattern 'class Module($_):
$$$'
ast-grep --pattern 'class Appliance($_):
$$$'
rg -nP 'iter_services' --type=py
rg -nP '_get_module_data\(' --type=py -C1Repository: plugwise/python-plugwise
Length of output: 4463
🏁 Script executed:
#!/bin/bash
# 1. Show the actual _get_module_data function signature and full implementation
sed -n '246,281p' plugwise/common.py
# 2. Check DomainObjects.appliance type
rg -n 'class DomainObjects' --type=py -A 30 | head -50
# 3. Search for iter_services definition
rg -n 'def iter_services|\.iter_services' --type=py -B 2 -A 5Repository: plugwise/python-plugwise
Length of output: 3560
🏁 Script executed:
#!/bin/bash
# 1. Check the exact calls in helper.py with more context
sed -n '145,160p' plugwise/helper.py
sed -n '225,240p' plugwise/helper.py
# 2. Check legacy/helper.py call
sed -n '205,220p' plugwise/legacy/helper.py
# 3. Search for iter_services definition
rg -n 'iter_services' --type=py -B 5 -A 10
# 4. Check if Appliance has services attribute (full class definition)
rg -n 'class Appliance' --type=py -A 40Repository: plugwise/python-plugwise
Length of output: 8475
🏁 Script executed:
#!/bin/bash
# Check the dead code block mentioned (lines 282-299)
sed -n '282,299p' plugwise/common.py
# Check if there's any working version or if this entire function needs rewriting
rg -n 'services.iter_services\|\.iter_services' --type=py -C 3
# Check git history to see if this was recently changed
git log --oneline -20 plugwise/common.py | head -20Repository: plugwise/python-plugwise
Length of output: 1031
_get_module_data has critical signature/logic mismatch: callers pass appliance objects, but the function signature accepts only key: str, and the implementation treats self.data.appliance (a list[Appliance]) as a single object.
From plugwise/model.py line 368: appliance: list[Appliance] = []. The function does:
"services" not in self.data.appliance— membership test on a list, alwaysFalse, preventing the guard clause from working.self.data.appliance.services.iter_services()—Appliancehas noservicesattribute;iter_services()method does not exist anywhere in the codebase.- Callers (
helper.pylines 152, 233;legacy/helper.pyline 212) pass appliance/location objects as the first positional argument, which currently lands in thekey: str | Noneparameter, causing a type error.
Option (a): Add appliance: Appliance parameter to the function signature and iterate its services (if it had one). Option (b): Iterate self.data.appliance and select the correct appliance by matching module services to the key filter.
Also remove the commented-out legacy code block (lines 282–299); it's unreachable dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/common.py` around lines 246 - 281, The _get_module_data function
currently has the wrong signature and treats self.data.appliance as a single
Appliance; change its signature to accept an appliance parameter (e.g., def
_get_module_data(self, appliance: Appliance, key: str | None = None, legacy:
bool = False) -> ModuleData) and update the body to operate on that appliance
(check appliance.services, iterate appliance.services.iter_services() or the
actual service iterable, call self.data.get_module(service.id) as before,
populate ModuleData and call get_zigbee_data(module, module_data, legacy));
update callers that pass an appliance to pass it into the new first argument;
alternatively if you prefer the other approach, iterate self.data.appliance (the
list) and find the matching Appliance by service/module when key is provided;
finally remove the unreachable commented-out legacy block after the function.
Comment on lines
103
to
+109
| 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.
"notification" in self.data won't do what you think on a Pydantic model.
self.data is a DomainObjects Pydantic BaseModel. Pydantic v2's BaseModel.__iter__ yields (field_name, value) tuples, so the in-check compares the string "notification" against (field_name, value) tuples and is always False — meaning _get_plugwise_notifications() is effectively never invoked through this guard.
Drop the membership check and rely on the None guard alone (notification is a declared field so it always exists as an attribute):
🩹 Proposed fix
- if "notification" in self.data and self.data.notification is not None:
- # print(f"HOI3b {self.data.notification}")
- self._get_plugwise_notifications()
+ if self.data.notification is not None:
+ self._get_plugwise_notifications()Also worth noting: DomainObjects.notification is Notification | list[Notification] | None, so _get_plugwise_notifications() must normalize the singleton case before iterating (previously flagged in helper.py).
📝 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
| 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() | |
| async def full_xml_update(self) -> None: | |
| """Perform a first fetch of the Plugwise server XML data.""" | |
| await self._request(DOMAIN_OBJECTS, new=True) | |
| # print(f"HOI3a {self.data}") | |
| if self.data.notification is not None: | |
| self._get_plugwise_notifications() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/smile.py` around lines 103 - 109, In full_xml_update(), remove the
incorrect membership test ("notification" in self.data) since self.data is a
Pydantic BaseModel; instead rely solely on the None check (if
self.data.notification is not None) to call _get_plugwise_notifications(); while
modifying _get_plugwise_notifications(), ensure it accepts
DomainObjects.notification which can be Notification | list[Notification] | None
and normalize a singleton Notification into an iterable (e.g., wrap non-list
values into a list) before iterating so the code handles both single and list
notifications (see DomainObjects.notification and helper.py for prior notes).
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"][4] | ||
| # 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.
Fix _parse_xml return-type annotation and drop the leftover debug block.
The function is annotated -> dict but actually returns a PlugwiseData (a Pydantic model). The commented-out block (lines 57–66) should also be removed — Sonar already flags it and it obscures the actual parsing step.
🩹 Proposed fix
- 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"][4]
- # 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)
+ def _parse_xml(self, xml: str) -> PlugwiseData:
+ """Map XML to Pydantic PlugwiseData."""
+ element = etree.fromstring(xml)
+ xml_dict = xmltodict.parse(etree.tostring(element))
+ return PlugwiseData.model_validate(xml_dict)🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 57-57: Remove this commented out code.
🤖 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 is
annotated to return dict but actually returns a PlugwiseData Pydantic model and
still contains a leftover commented debug block; update the signature of
_parse_xml to return PlugwiseData instead of dict and remove the entire
commented-out debug block (the print/xml-to-json/appliance examples) so the
method contains only the parsing steps (etree.fromstring, xmltodict.parse, and
return PlugwiseData.model_validate(xml_dict)); keep references to PlugwiseData
and the existing xml parsing calls unchanged.
Comment on lines
+172
to
177
| if new: | ||
| domain_objects = result | ||
| root = self._parse_xml(domain_objects) | ||
| self.data = root.domain_objects | ||
| return result | ||
| return xml |
Contributor
There was a problem hiding this comment.
self.data is set on SmileComm but declared on the Smile subclass — tighten the contract.
SmileComm never declares self.data, so assigning self.data = root.domain_objects here relies on the Smile subclass declaring it (plugwise/__init__.py line 79). This will silently break if someone instantiates SmileComm directly (or introduces another subclass) and, more subtly, the annotation on Smile.data is PlugwiseData while the value stored is actually a DomainObjects (root.domain_objects). That contradicts the annotation and every downstream caller (self.data.appliance, self.data.module, self.data.gateway, etc.) — those work only because self.data is actually a DomainObjects.
Please pick one and make it consistent:
- store
self.data = rootand rewrite callers asself.data.domain_objects.*, or - store
self.data = root.domain_objectsand fix the annotation inplugwise/__init__.pytoself.data: DomainObjects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/smilecomm.py` around lines 172 - 177, Assign and treat self.data as
DomainObjects: keep the assignment in SmileComm (self.data =
root.domain_objects) but make the contract explicit by updating the Smile class
annotation from PlugwiseData to DomainObjects and add a declaration on the base
SmileComm (e.g., declare self.data: DomainObjects = None or a class attribute)
so the attribute is defined on the base class and type-consistent for all
subclasses; update any imports/typing to reference DomainObjects where needed.
Comment on lines
+79
to
+87
| def check_heater_central(data: PlugwiseData) -> str: | ||
| """Find the valid heater_central, helper-function for _appliance_info_finder(). | ||
|
|
||
| Solution for Core Issue #104433, | ||
| for a system that has two heater_central appliances. | ||
| """ | ||
| locator = "./appliance[type='heater_central']" | ||
| heater_central_count = 0 | ||
| heater_central_list: list[dict[str, bool]] = [] | ||
| for heater_central in xml.findall(locator): | ||
| if (heater_central_id := heater_central.get("id")) is None: | ||
| continue # pragma: no cover | ||
|
|
||
| if (heater_central_name := heater_central.find("name")) is None: | ||
| continue # pragma: no cover | ||
|
|
||
| has_actuators = heater_central.find("actuator_functionalities/") is not None | ||
| # Filter for Plug/Circle/Stealth heater_central -- Pw-Beta Issue #739 | ||
| if heater_central_name.text == "Central heating boiler": | ||
| heater_central_list.append({heater_central_id: has_actuators}) | ||
| heater_central_count += 1 | ||
| for appliance in data.appliance: |
Contributor
There was a problem hiding this comment.
Type annotation contradicts actual caller usage — data is a DomainObjects, not PlugwiseData.
plugwise/smilecomm.py line 175 assigns self.data = root.domain_objects (a DomainObjects), and plugwise/common.py line 93 calls check_heater_central(self.data). The iteration for appliance in data.appliance only works because DomainObjects.appliance is the list — PlugwiseData exposes .domain_objects, not .appliance, so the current annotation would fail at runtime if anyone actually passed a PlugwiseData.
Fix the annotation (and the import) to match reality:
🩹 Proposed fix
-from plugwise.model import PlugwiseData
+from plugwise.model import DomainObjects
@@
-def check_heater_central(data: PlugwiseData) -> str:
+def check_heater_central(data: DomainObjects) -> str:📝 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
| def check_heater_central(data: PlugwiseData) -> str: | |
| """Find the valid heater_central, helper-function for _appliance_info_finder(). | |
| Solution for Core Issue #104433, | |
| for a system that has two heater_central appliances. | |
| """ | |
| locator = "./appliance[type='heater_central']" | |
| heater_central_count = 0 | |
| heater_central_list: list[dict[str, bool]] = [] | |
| for heater_central in xml.findall(locator): | |
| if (heater_central_id := heater_central.get("id")) is None: | |
| continue # pragma: no cover | |
| if (heater_central_name := heater_central.find("name")) is None: | |
| continue # pragma: no cover | |
| has_actuators = heater_central.find("actuator_functionalities/") is not None | |
| # Filter for Plug/Circle/Stealth heater_central -- Pw-Beta Issue #739 | |
| if heater_central_name.text == "Central heating boiler": | |
| heater_central_list.append({heater_central_id: has_actuators}) | |
| heater_central_count += 1 | |
| for appliance in data.appliance: | |
| from plugwise.model import DomainObjects | |
| def check_heater_central(data: DomainObjects) -> str: | |
| """Find the valid heater_central, helper-function for _appliance_info_finder(). | |
| Solution for Core Issue `#104433`, | |
| for a system that has two heater_central appliances. | |
| """ | |
| heater_central_count = 0 | |
| heater_central_list: list[dict[str, bool]] = [] | |
| for appliance in data.appliance: |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 79-79: Refactor this function to reduce its Cognitive Complexity from 19 to the 15 allowed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugwise/util.py` around lines 79 - 87, The type annotation for the
check_heater_central function is incorrect: update the parameter type from
PlugwiseData to DomainObjects (and import DomainObjects) because callers pass
root.domain_objects; adjust the function signature for
check_heater_central(data: DomainObjects) and update the import list to include
DomainObjects so iteration over data.appliance matches the actual object shape
used by smilecomm.py and common.py.
Comment on lines
+19
to
+30
| # @pytest.mark.asyncio | ||
| # async def test_connect_adam_bad_thermostat(self, caplog): | ||
| # """Test Adam with missing thermostat data.""" | ||
| # self.smile_setup = "adam_bad_thermostat" | ||
| # | ||
| # testdata = await self.load_testdata(SMILE_TYPE, self.smile_setup) | ||
| # server, api, client = await self.connect_wrapper(raise_timeout=True) | ||
| # await self.device_test(api, "2023-12-17 00:00:01", testdata) | ||
| # assert "Thermostat data in Zone" in caplog.text | ||
| # | ||
| # await api.close_connection() | ||
| # await self.disconnect(server, client) |
Contributor
There was a problem hiding this comment.
Don't leave a commented-out test behind — restore or delete with rationale.
The test_connect_adam_bad_thermostat body is commented out instead of removed or re-enabled. This loses coverage for the "Thermostat data in Zone" warning path in _regulation_control (helper.py ~line 863), which is still live code. Either:
- restore the test once the model-based helper path emits the same warning, or
- delete it outright with a PR note if the scenario is no longer reachable after the refactor.
Leaving commented source in the committed tree makes it unclear whether the behavior is intentionally unverified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_adam.py` around lines 19 - 30, The commented-out
test_connect_adam_bad_thermostat should be resolved: either restore and
re-enable the test (uncomment, add pytest.mark.asyncio, ensure test uses
smile_setup "adam_bad_thermostat" and asserts the "Thermostat data in Zone"
warning emitted by _regulation_control) if that warning path is still reachable,
or remove the commented block entirely and add a PR note explaining why the
scenario is no longer reachable after the refactor; reference
test_connect_adam_bad_thermostat, the asserted string "Thermostat data in Zone",
and the helper function _regulation_control when making the change.
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
Refactor
Bug Fixes