Further code optimizations #838
Conversation
WalkthroughAdds a THERMO_MATCHING constant and refactors appliance/location helpers: renames methods, extends appliance data and plug-class logic, changes thermostat-ranking signatures and matching to mutate internal _loc_data, and makes thermostat scanning run in the thermostat path (removed ADAM-only guard). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2024-12-22T09:37:24.648ZApplied to files:
📚 Learning: 2025-12-21T10:56:14.461ZApplied to files:
📚 Learning: 2025-10-11T14:05:29.022ZApplied to files:
🔇 Additional comments (5)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #838 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3442 3435 -7
=========================================
- Hits 3442 3435 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise/constants.py (1)
95-100: Consider usingFinaltype annotation for immutability.The
THERMO_MATCHINGconstant should be annotated withFinal[dict[str, int]]to match the pattern of other module-level constants and to clearly indicate this mapping should not be modified at runtime.🔎 Proposed fix
-THERMO_MATCHING: dict[str, int] = { +THERMO_MATCHING: Final[dict[str, int]] = { "thermostat": 2, "zone_thermometer": 2, "zone_thermostat": 2, "thermostatic_radiator_valve": 1, }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/constants.py(1 hunks)plugwise/helper.py(4 hunks)plugwise/smile.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (2)
plugwise/smile.py (1)
plugwise/helper.py (3)
_get_appliances(97-159)_get_appliances_with_offset_functionality(300-309)_scan_thermostats(742-764)
plugwise/helper.py (2)
plugwise/common.py (1)
_create_gw_entities(147-166)plugwise/constants.py (2)
GwEntityData(533-595)ThermoLoc(511-517)
🔇 Additional comments (11)
plugwise/smile.py (2)
109-109: LGTM - Method rename aligns with helper module.The rename from
_all_appliances()to_get_appliances()is consistent with the refactored method name inplugwise/helper.py.
110-114: LGTM - Simplified conditional logic with internal gating.The thermostat scanning is now called unconditionally when
_is_thermostatis true, which simplifies this call site. The ADAM-specific gating has been moved inside_scan_thermostats()itself (lines 748-749 inplugwise/helper.py), which is a cleaner separation of concerns.plugwise/helper.py (9)
97-104: LGTM - Method rename improves clarity.The rename from
_all_appliancesto_get_appliancesbetter reflects that this method both collects and processes appliances, rather than just returning all of them. The updated docstring accurately describes the method's behavior.
108-120: LGTM - Explicit initialization improves code clarity.Explicitly initializing all appliance attributes to
Noneor appropriate default values makes the data structure more predictable and easier to understand. This is a good practice for avoiding potentialAttributeErrorexceptions.
122-128: LGTM - Clearer comments and consistent filtering logic.The updated comment better explains the filtering logic for OpenThermGateway appliances and orphaned thermostats. The multi-line condition format improves readability.
130-146: LGTM - Location assignment logic is more explicit.The refactored location assignment logic with clearer comments makes the handling of appliances without locations more understandable. The plug description handling remains functionally equivalent.
154-159: LGTM - Improved code organization.Moving the P1 smartmeter collection after appliance processing and explicitly calling
_reorder_devices()makes the workflow more obvious and maintainable.
742-764: LGTM - Adam-specific logic properly guarded.The updated
_scan_thermostats()method correctly includes an early return for non-ADAM devices, and the refactored iteration over_loc_datais cleaner. The docstring accurately reflects that this is Adam-only functionality.
766-775: LGTM - Method rename reflects updated behavior.The rename from
_match_locationsto_match_and_rank_thermostatsbetter describes the method's purpose. The refactored implementation that directly populates internal structures rather than returning a dictionary is a cleaner design.
777-810: Parameter order change is correct and consistent throughout codebase.The caller at line 773-775 passes arguments in the correct order matching the new signature:
entity_id, entity, location_id, location, thermo_matching. Only one call to this method exists, and it has been updated correctly.
197-214: Code structure is correct —_loc_datainitialization matches theThermoLocTypedDict.The initialization in
_get_locations()(lines 204-210) creates dictionaries with all required keys (name,primary,primary_prio,secondary) matching theThermoLocstructure defined inplugwise/constants.py:511-517. All usages of_loc_datain the codebase access compatible keys. The_home_location(XML element) and_home_loc_id(string ID) serve distinct purposes and are used appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugwise/helper.py (2)
106-164: LGTM - Clean refactoring of appliance collection.The rename from
_all_appliances()to_get_appliances()aligns with the PR objectives. The reorganization improves clarity by:
- Initializing all appliance fields upfront (lines 117-129)
- Extracting plug device class logic to
extend_plug_device_class()(line 150)- Maintaining the skip logic and flow control
Optional: Consider consolidating field initialization
Lines 117-129 could be more concise using a dict:
appl = Munch({ "available": None, "entity_id": appliance.attrib["id"], "location": None, "name": appliance.find("name").text, "model": None, "model_id": None, "module_id": None, "firmware": None, "hardware": None, "mac": None, "pwclass": appliance.find("type").text, "zigbee_mac": None, "vendor_name": None, })This is purely stylistic and the current approach is equally valid.
770-779: Consider the efficiency of nested location × entity loops.Lines 775-779 iterate over all locations and all entities (O(n×m) complexity). If the number of locations and entities is large, this could be inefficient.
Consider building a location-to-entities map once:
def _match_and_rank_thermostats(self) -> None: """Match thermostat-appliances with locations, rank them for locations with multiple thermostats.""" # Build location index entities_by_location: dict[str, list[tuple[str, GwEntityData]]] = {} for entity_id, entity in self.gw_entities.items(): if "location" in entity: loc = entity["location"] entities_by_location.setdefault(loc, []).append((entity_id, entity)) # Rank thermostats per location for location_id, location in self._loc_data.items(): for entity_id, entity in entities_by_location.get(location_id, []): self._rank_thermostat(entity_id, entity, location_id, location, THERMO_MATCHING)This would reduce complexity to O(n+m). However, if the dataset sizes are small (typical for home automation), the current approach is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (3)
plugwise/legacy/helper.py (1)
_appliance_info_finder(192-205)plugwise/common.py (2)
_create_gw_entities(147-166)check_name(67-72)plugwise/constants.py (2)
GwEntityData(533-595)ThermoLoc(511-517)
🔇 Additional comments (5)
plugwise/helper.py (5)
201-218: LGTM - Well-structured location data initialization.The rename and refactoring of
_get_locations()cleanly initializes the_loc_datastructure that supports the new thermostat ranking logic. The dict structure with"name","primary","primary_prio", and"secondary"keys aligns with theThermoLocTypedDict.
746-768: LGTM - Thermostat scanning refactored correctly.The refactored
_scan_thermostats()maintains the Adam-only guard and now uses_match_and_rank_thermostats()to populate_loc_datawith thermostat rankings. The iteration over_loc_data(lines 756-768) correctly builds zone entities from the ranking results.
781-815: LGTM - Thermostat ranking logic is sound.The updated
_rank_thermostat()signature and implementation correctly:
- Matches thermostats to locations by comparing
entity["location"]withlocation_id- Ranks based on
thermo_matchingpriority values- Handles demotion of former primary thermostats to secondary (lines 808-810)
- Supports multiple primary thermostats with the same priority (line 801-802)
The logic flow is clear and handles the edge cases appropriately.
33-33: THERMO_MATCHING constant is properly defined.The imported
THERMO_MATCHINGconstant is correctly defined inplugwise/constants.py(line 95) as aFinal[dict[str, int]]mapping device class strings to priority integers:
- "thermostat": 2
- "zone_thermometer": 2
- "zone_thermostat": 2
- "thermostatic_radiator_valve": 1
The import at line 33 and usage at line 778 are both correct.
781-815: No action required. Thedev_classkey is always initialized in_create_gw_entities()(line 149 in common.py:self.gw_entities[appl.entity_id] = {"dev_class": appl.pwclass}), and all entities passed to_rank_thermostat()come fromself.gw_entities.items()after creation. There are no code paths that add entries togw_entitieswithout this key.
bouwew
force-pushed
the
correct-group-related
branch
from
57876ad to
6a41486
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugwise/helper.py (1)
805-809: Optional: Remove redundant location check.The condition
location_id == entity["location"]at line 807 is redundant because we're already iterating overentities_by_location.get(location_id, []), which only contains entities whereentity["location"]matcheslocation_id. While this defensive check doesn't hurt, it could be simplified for clarity.🔎 Proposed simplification
- if ( - "location" in entity - and location_id == entity["location"] - and appl_class in thermo_matching - ): + if "location" in entity and appl_class in thermo_matching:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/helper.py(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Process test coverage
🔇 Additional comments (4)
plugwise/helper.py (4)
60-69: Past review concern properly addressed.The AttributeError risk flagged in the previous review has been correctly resolved. The code now properly guards against missing
descriptionelements using walrus operators before accessing.text.
108-166: LGTM!The renaming to
_get_appliances()follows a clearer naming convention, and the additional field initializations (zigbee_mac, vendor_name) along with the call toextend_plug_device_class()properly extend the appliance data structure.
748-771: LGTM!The Adam-only guard at the start is a good defensive check, and the updated code properly uses the new location data structure (
location["primary_prio"],location["primary"],location["secondary"]).
772-789: The refactoring to build anentities_by_locationindex and mutate_loc_datadirectly is sound. TheTHERMO_MATCHINGconstant is properly defined inplugwise.constantswith the expected thermostat priority mappings.
plugwise/helper.py
Outdated
| # Build location index | ||
| entities_by_location: dict[str, list[tuple[str, GwEntityData]]] = {} | ||
| for entity_id, entity in self.gw_entities.items(): | ||
| if "location" in entity: | ||
| loc = entity["location"] | ||
| entities_by_location.setdefault(loc, []).append((entity_id, entity)) | ||
|
|
||
| # Rank thermostats per location | ||
| for location_id, location in self._loc_data.items(): | ||
| for entity_id, entity in entities_by_location.get(location_id, []): | ||
| self._rank_thermostat( | ||
| entity_id, entity, location_id, location, THERMO_MATCHING | ||
| ) |
There was a problem hiding this comment.
Wondered if we couldn't do this in a single loop
| # Build location index | |
| entities_by_location: dict[str, list[tuple[str, GwEntityData]]] = {} | |
| for entity_id, entity in self.gw_entities.items(): | |
| if "location" in entity: | |
| loc = entity["location"] | |
| entities_by_location.setdefault(loc, []).append((entity_id, entity)) | |
| # Rank thermostats per location | |
| for location_id, location in self._loc_data.items(): | |
| for entity_id, entity in entities_by_location.get(location_id, []): | |
| self._rank_thermostat( | |
| entity_id, entity, location_id, location, THERMO_MATCHING | |
| ) | |
| # Rank thermostats by walking locations | |
| for location_id, location in self._loc_data.items(): | |
| for entity_id, entity in self.gw_entities.items(): | |
| if entity.get("location") == location_id: | |
| self._rank_thermostat( | |
| entity_id, entity, location_id, location, THERMO_MATCHING | |
| ) |
There was a problem hiding this comment.
Yes I had that. See the nitpick above, the code-change comes from the suggestion in the nitpick.
And it has more benefits, it simplifies the _rank_thermostat() function.
plugwise/helper.py
Outdated
| appl_class = entity["dev_class"] | ||
| if ( | ||
| "location" in entity | ||
| and location_id == entity["location"] | ||
| and appl_class in thermo_matching | ||
| ): |
There was a problem hiding this comment.
Suggest turning this negative and return None
There was a problem hiding this comment.
This line has been simplified, see the latest commits.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/helper.py(5 hunks)plugwise/legacy/helper.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
📚 Learning: 2024-12-22T09:37:24.648Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 678
File: plugwise/legacy/helper.py:69-69
Timestamp: 2024-12-22T09:37:24.648Z
Learning: The code in plugwise/legacy/helper.py is fully separated from plugwise/helper.py. The user indicates that the problem signaled about the mismatch of the variable name (_home_location vs _home_loc_id) does not actually apply.
Applied to files:
plugwise/helper.pyplugwise/legacy/helper.py
📚 Learning: 2025-10-11T14:05:29.022Z
Learnt from: bouwew
Repo: plugwise/python-plugwise PR: 797
File: fixtures/adam_anna_new_2/data.json:1-304
Timestamp: 2025-10-11T14:05:29.022Z
Learning: In Plugwise fixture data (data.json files), location IDs referenced in device entries don't always need corresponding top-level location objects. Only climate zones (ThermoZone entries with dev_class "climate") require full top-level definitions with thermostats, schedules, and presets. Simple organizational locations that just group devices (like single plugs) can be referenced without top-level entries.
Applied to files:
plugwise/helper.pyplugwise/legacy/helper.py
🧬 Code graph analysis (1)
plugwise/helper.py (3)
plugwise/legacy/helper.py (1)
_appliance_info_finder(189-202)plugwise/common.py (2)
_create_gw_entities(147-166)check_name(67-72)plugwise/constants.py (2)
GwEntityData(533-595)ThermoLoc(511-517)
🔇 Additional comments (5)
plugwise/helper.py (5)
60-68: LGTM!The defensive null checks properly guard against
AttributeErrorwhen thedescriptionelement or its text is missing. This addresses the past review concern.
108-166: LGTM!The refactoring improves clarity with the
_get_prefix, explicit field initialization, and extracted helper function. The logic flow remains intact.
226-229: Verify intentional removal of fallback for missing home location.The code now raises
KeyErrorwhen no building location is found, whereas a past review noted that the legacy implementation creates a fake "Home" location as a fallback. This represents a breaking behavioral change that could cause initialization failures in edge cases.Confirm this fail-fast approach is intentional and that all supported configurations guarantee a building-type location exists.
Based on past review comments indicating fallback handling was needed for missing "Home" location.
757-779: LGTM!The refactoring cleanly integrates the new thermostat ranking results from
_match_and_rank_thermostats()into the zone creation logic. The Adam-only guard remains in place as appropriate.
781-828: LGTM!The refactored thermostat matching and ranking logic is well-structured. Building the entity-by-location index improves clarity, and the in-place mutation of location data aligns with the new
_loc_datastructure. The ranking algorithm correctly handles primary election and demotion to secondary.
plugwise/helper.py
Outdated
| if loc_id == appl_d_loc and appl_class in thermo_matching: | ||
| if thermo_matching[appl_class] == thermo_loc["primary_prio"]: | ||
| thermo_loc["primary"].append(appliance_id) | ||
| if (appl_class := entity["dev_class"]) in thermo_matching: |
There was a problem hiding this comment.
Same still applies, negate and return as there is no 'else'
|
Changes:
_all_...to_get_..._get_appliances()_scan_thermostat()and related functionsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.