Improve/extend processing of Groups #828
Conversation
to solve filesize warnings
|
Warning Rate limit exceeded@bouwew has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughGeneralize group handling: introduce GROUP_TYPES and GROUP_MEASUREMENTS, rename/internalize group retrieval to _get_groups, add group sensor collection, update fixtures/tests with new Group entries and sensors, bump project version and changelog. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test / Caller
participant H as SmileHelper._get_measurement_data()
participant C as SmileHelper._collect_group_sensors()
participant S as Data Source (group logs)
participant D as Internal datastore
T->>H: request measurements for entity
H->>H: inspect entity for "members"
alt entity has members (Group)
H->>C: collect GROUP_MEASUREMENTS for entity_id
C->>S: locate group logs (by id / XPath)
loop each measurement
C->>S: query measurement logs
C->>D: apply matching & increment counters
end
C-->>H: return collected measurements
else not a group
H-->>H: collect normal device measurements
end
H-->>T: return measurement data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
biome.json(1 hunks)fixtures/adam_multiple_devices_per_zone/data.json(2 hunks)fixtures/adam_plus_anna_new/data.json(2 hunks)fixtures/adam_plus_anna_new_regulation_off/data.json(2 hunks)fixtures/adam_zone_per_device/data.json(1 hunks)fixtures/m_adam_cooling/data.json(2 hunks)fixtures/m_adam_heating/data.json(2 hunks)fixtures/m_adam_multiple_devices_per_zone/data.json(2 hunks)fixtures/stretch_v23/data.json(1 hunks)fixtures/stretch_v31/data.json(2 hunks)plugwise/common.py(3 hunks)plugwise/constants.py(2 hunks)plugwise/helper.py(4 hunks)plugwise/legacy/smile.py(1 hunks)plugwise/smile.py(1 hunks)tests/data/adam/adam_multiple_devices_per_zone.json(2 hunks)tests/data/adam/adam_plus_anna_new.json(2 hunks)tests/data/adam/adam_plus_anna_new_regulation_off.json(2 hunks)tests/data/adam/adam_zone_per_device.json(1 hunks)tests/data/stretch/stretch_v23.json(1 hunks)tests/data/stretch/stretch_v31.json(2 hunks)tests/test_adam.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
fixtures/m_adam_heating/data.jsonfixtures/adam_zone_per_device/data.jsonfixtures/adam_multiple_devices_per_zone/data.jsonfixtures/stretch_v31/data.jsonfixtures/stretch_v23/data.jsontests/data/adam/adam_plus_anna_new_regulation_off.jsonfixtures/m_adam_cooling/data.jsonfixtures/m_adam_multiple_devices_per_zone/data.jsontests/data/adam/adam_zone_per_device.jsonfixtures/adam_plus_anna_new_regulation_off/data.jsontests/data/adam/adam_multiple_devices_per_zone.jsontests/data/adam/adam_plus_anna_new.jsonfixtures/adam_plus_anna_new/data.json
🧬 Code graph analysis (3)
plugwise/smile.py (1)
plugwise/common.py (1)
_get_groups(191-222)
plugwise/legacy/smile.py (1)
plugwise/common.py (1)
_get_groups(191-222)
plugwise/helper.py (2)
plugwise/constants.py (1)
GwEntityData(527-589)plugwise/util.py (1)
common_match_cases(146-172)
🪛 GitHub Actions: Latest commit
biome.json
[error] 5-5: Biome formatting check failed. Formatter would have printed changes to biome.json. Run 'pre-commit run --show-diff-on-failure --color=always --all-files --hook-stage manual biome' to fix formatting.
🪛 GitHub Check: SonarCloud Code Analysis
plugwise/helper.py
[warning] 380-380: Remove the unused function parameter "entity".
🔇 Additional comments (31)
fixtures/adam_zone_per_device/data.json (1)
512-524: Fixture structure looks correct for the new group entity.The new pumping group entry is properly structured with valid member references (both devices exist in the fixture) and appropriate sensor aggregation. As noted in the learnings, group-level entities don't require top-level location definitions, which aligns with the fixture design here.
fixtures/adam_plus_anna_new_regulation_off/data.json (2)
158-172: Verify temperature sensor aggregation for Vloerverwarming group.The temperature sensor value (18.4) does not match the Anna thermostat member device (which reads 22.4°C). Clarify whether this is intentional (e.g., aggregated from a different source, setpoint vs. actual temperature) or if the fixture data should be corrected to align with member device readings.
222-237: Verify electricity aggregation for Test group.The group's
electricity_consumedsensor shows 14.8, which matches only the first member (Plug MediaTV). The second member, Plug Werkplek, has 91.3W consumption. Confirm whether group sensor aggregation should sum member values (≈106.1W total) or if using only a subset is the intended design. If intentional, document the aggregation strategy to avoid confusion.fixtures/stretch_v23/data.json (1)
305-313: Group model rename matches new group handlingChanging
"model": "Switchgroup"to"model": "Group"forf7b145c8...aligns this Stretch fixture with the new_get_groupsoutput (which always setsmodelto"Group"). No structural issues spotted.fixtures/m_adam_multiple_devices_per_zone/data.json (2)
508-520: New pumping group entry looks consistent
e117db6848...usesdev_class: "pumping",model: "Group", and amemberslist pointing at existing devices, with asensors.electricity_consumedvalue that matches nearby data. This matches the newGROUP_TYPES/GROUP_MEASUREMENTSmodel and doesn’t introduce location inconsistencies.Based on learnings, group entries can omit their own location when only aggregating member devices.
549-565: Switching group converted to Group with sensors is structurally OKFor
e8ef2a01ed3b4139a53bf749204fe6b4, setting"model": "Group"and adding asensorsblock withelectricity_consumed/electricity_producedmatches the new group‑sensor design and uses valid measurement keys.Based on learnings, using members plus sensors here without extra location metadata is acceptable.
plugwise/constants.py (2)
109-113: GROUP_MEASUREMENTS integrates cleanly with existing measurement mapsThe new
GROUP_MEASUREMENTSdict mirrors the existing P1/DEVICE/ZONE measurement structures and uses already‑defined sensor keys and units, so it should plug into_collect_group_sensorswithout type or key issues.
307-308: GROUP_TYPES definition matches current group fixtures and helper logicDefining
GROUP_TYPES = ("pumping", "report", "switching")lines up with the dev_class values used in the fixtures and the_get_groupshelper inplugwise/common.py. This should generalize group handling beyond just switch/report groups.fixtures/stretch_v31/data.json (2)
88-100: Stretch v31 “Schakel” group model rename is consistentUpdating
"model"to"Group"ford03738ed...matches the new group entity model while keeping dev_class/members intact. Looks correct.
101-116: Stretch v31 “Stroomvreters” group model rename matches new semanticsSame as above:
"model": "Group"ford950b314...aligns with the generalized group handling; no structural or data issues seen.plugwise/legacy/smile.py (1)
77-88: Legacy gateway now using unified _get_groups helperSwitching
get_all_gateway_entities()to useself._get_groups()ensures legacy devices follow the same group discovery logic as the common code (including pumping/report/switching groups). The conditional update intogw_entitieslooks fine.tests/data/stretch/stretch_v23.json (1)
305-313: Test snapshot updated to match new group modelChanging the test data model of
f7b145c8...to"Group"keeps this snapshot aligned with the fixture and the_get_groupsimplementation.tests/data/adam/adam_plus_anna_new_regulation_off.json (2)
181-190: New pumping Group entity fits the extended group model
c9293d1d68ee48fc8843c6f0dee2b6beis defined as adev_class: "pumping"group with valid member IDs andmodel: "Group", matching the new group semantics and what_get_groupswould construct. Omittingsensors/switcheshere is consistent if this group is sensor‑only in this scenario.Based on learnings, a separate location object isn’t required since this simply aggregates existing devices.
217-229: Switching group model rename in Adam+Anna test data is correctUpdating
"model": "Group"fore8ef2a01ed3b4139a53bf749204fe6b4keeps this test dataset in sync with the new group modeling while preserving existing members and switch behavior.plugwise/smile.py (1)
118-119: LGTM! Method call updated correctly.The change from
_get_group_switches()to_get_groups()aligns with the refactoring in plugwise/common.py that generalizes group handling beyond just switching groups.fixtures/adam_plus_anna_new/data.json (2)
184-198: LGTM! New pumping group with aggregated sensors.The new group entity correctly aggregates sensor data from its members (Plug Vloerverwarming's electricity_consumed and Anna's temperature).
269-285: LGTM! Group model updated with aggregated sensors.The switching group has been updated from "Switchgroup" to "Group" model and now includes aggregated electricity sensors from its members (16.5 ≈ 15.8 + 0.69).
plugwise/common.py (1)
191-222: LGTM! Generalized group handling implementation.The refactoring successfully generalizes group handling:
- Method renamed to reflect broader scope (_get_groups vs _get_group_switches)
- Uses GROUP_TYPES constant for flexible filtering
- Model field updated to "Group" for consistency
- Internal variable naming improved (groups vs switches)
fixtures/m_adam_cooling/data.json (2)
94-108: LGTM! Pumping group consistent with other fixtures.The new pumping group follows the same pattern as in adam_plus_anna_new, with appropriate sensor aggregation.
185-201: LGTM! Switching group updated consistently.Model and sensors updated to match the pattern in other Adam fixtures.
tests/test_adam.py (3)
37-56: LGTM! Test expectations updated for new group entity.The entity count increase from 220 to 230 (+10) correctly accounts for the new pumping group entity (5 fields) and sensors added to the existing switching group (5 fields). The new group ID is properly added to the entity list.
231-232: LGTM! Entity count updated for adam_zone_per_device.The increase from 379 to 385 (+6) reflects the addition of group-related entities in the corresponding fixture.
305-306: LGTM! Entity count updated for adam_multiple_devices_per_zone.The increase from 385 to 393 (+8) accounts for new group entities and sensor additions in the fixture.
tests/data/adam/adam_plus_anna_new.json (2)
184-198: LGTM! Test data matches fixture.The new pumping group in test data is consistent with the corresponding fixture file.
269-285: LGTM! Test data matches fixture.The updated switching group in test data matches the fixture file.
fixtures/m_adam_heating/data.json (2)
99-113: LGTM! Heating fixture updated consistently.The new pumping group follows the established pattern across all Adam fixtures.
184-200: LGTM! Switching group updated consistently.The model and sensor updates match the pattern in other fixtures.
tests/data/stretch/stretch_v31.json (1)
88-116: Based on my verification, I can now provide the rewritten review comment. The investigation reveals that:
- Stretch groups in the XML fixture have
point_logdefinitions but contain NO<period><measurement>data- The
_collect_group_sensorsfunction only adds sensors if it finds actual measurement data in the XML- Adam groups DO have
<period><measurement>data in their XML, which is why their JSON fixtures include sensors- Stretch groups lack
temperaturepoint_log entries entirely, while Adam groups may have themThe absence of sensors in Stretch group JSON is correct and expected, not a bug.
Stretch groups correctly have no sensors block because their XML fixture lacks measurement data.
The XML for both Stretch groups (
d03738edfcc947f7b8f4573571d90d2dandd950b314e9d8499f968e6db8d82ef78c) definespoint_logelements forelectricity_consumedandelectricity_produced, but they are empty—containing no<period><measurement>child elements with actual values. The_collect_group_sensors()helper (line 377–391 inplugwise/helper.py) returns early if measurement data is missing (line 391), so no sensors are added to the JSON. This is correct behavior: the fixture accurately represents the source XML. Unlike Adam group fixtures, which have populated<period><measurement>data and thus includesensorsblocks in their JSON, Stretch groups have no measurement data to collect.fixtures/adam_multiple_devices_per_zone/data.json (1)
515-527: Group fixtures look consistent and aligned with member devicesThe new pumping group
e117db6848394c8cb70d9c28e63d92d2and the updated switching groupe8ef2a01ed3b4139a53bf749204fe6b4are structurally sound: member ids exist,dev_class/modelcombinations are coherent, andelectricity_consumedvalues are consistent with the underlying devices/zones. No issues from the fixture side.Also applies to: 556-567
tests/data/adam/adam_zone_per_device.json (1)
512-524: Zone-per-device test data for pumping group is coherentThe new pumping
Groupentitye117db6848394c8cb70d9c28e63d92d2correctly references the pump and thermostat members, andelectricity_consumedmatches the Woonkamer zone and pump readings in this scenario. This should exercise the new group-sensor path as intended.tests/data/adam/adam_multiple_devices_per_zone.json (1)
515-527: Multiple-devices-per-zone test groups correctly mirror fixture changesBoth the new pumping group and the updated switching group entries are consistent with the fixture data and with each other (ids, members, and sensor values). They provide good coverage for the extended group handling (pumping + switching with sensors).
Also applies to: 556-567
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_adam.py (1)
37-37: Test expectations updated consistently for new group entitiesThe higher
entity_itemscounts and the inclusion ofc9293d1d68ee48fc8843c6f0dee2b6bein theadam_plus_anna_newentity_listline up with the new Group entries and group‑sensor aggregation:
adam_plus_anna_new: 220 → 230 and explicit listing of the new group ID ensures the helper surfaces the new pumping/switch group entity.adam_zone_per_deviceandadam_multiple_devices_per_zone: counts increased in line with added group‑level sensors and entities.If you want slightly stronger regression protection around the new pumping group in the
adam_multiple_devices_per_zonefixture, you could also add an explicit assertion on the presence and sensor contents ofe117db6848394c8cb70d9c28e63d92d2(similar to how the plus‑Anna test asserts the new entity ID), but the current updates are already coherent with the underlying changes.Also applies to: 52-52, 231-231, 305-305
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
biome.json(1 hunks)fixtures/adam_multiple_devices_per_zone/data.json(2 hunks)fixtures/adam_zone_per_device/data.json(1 hunks)fixtures/m_adam_multiple_devices_per_zone/data.json(2 hunks)plugwise/helper.py(4 hunks)tests/data/adam/adam_multiple_devices_per_zone.json(2 hunks)tests/data/adam/adam_zone_per_device.json(1 hunks)tests/test_adam.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- biome.json
- fixtures/m_adam_multiple_devices_per_zone/data.json
- fixtures/adam_zone_per_device/data.json
- tests/data/adam/adam_multiple_devices_per_zone.json
🧰 Additional context used
🧠 Learnings (1)
📚 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:
fixtures/adam_multiple_devices_per_zone/data.jsontests/data/adam/adam_zone_per_device.json
⏰ 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: Run pytest using Python 3.13
🔇 Additional comments (4)
tests/data/adam/adam_zone_per_device.json (1)
512-525: Fixture entry is well-structured with valid member references.The new Group entry is properly formatted. Member IDs both exist in the fixture with matching sensor values (CV Pomp provides
electricity_consumed: 35.8; Zone Lisa WK providestemperature: 21.1), indicating the group aggregates from its members correctly. The minimal field set for a Group device aligns with the learning that groups don't require elaborate top-level definitions like climate zones.To ensure completeness, please verify that:
- Tests referencing this fixture expect the new Group entry to be present
- The GROUP_TYPES and GROUP_MEASUREMENTS constants in the code changes properly handle this group's
dev_class: pumpingand sensor keysfixtures/adam_multiple_devices_per_zone/data.json (2)
515-528: New pumping group entry looks consistent with group modelThe added
e117db6848394c8cb70d9c28e63d92d2group uses the expecteddev_class: "pumping",model: "Group",memberslist, and a leansensorsblock (electricity + temperature). Member IDs refer to existing devices in the same zone, and relying on member locations rather than a separate top‑level location aligns with earlier fixture patterns for non‑zone entities.
Based on learnings, the absence of a dedicated top‑level location for this group is fine.
563-568: Switch group migration to Group with sensors is coherentUpdating
e8ef2a01ed3b4139a53bf749204fe6b4tomodel: "Group"and addingsensors(electricity_consumed,electricity_produced) matches the new group‑handling path and GROUP_MEASUREMENTS expectations. Together with the existingmembersandswitches.relay, this should allow the helper to expose both aggregated power and group switch state cleanly.plugwise/helper.py (1)
24-24: Group sensor wiring and implementation look correctThe new GROUP_MEASUREMENTS import, the
"members"check in_get_measurement_data, and_collect_group_sensorstogether form a clean, self‑contained path for group entities:
- P1/smartmeter handling remains first and still short‑circuits for pure power Smiles, so group logic won’t interfere there.
- Detecting groups via
"members" in entityis straightforward and won’t affect normal appliances._collect_group_sensorsnow:
- Guards on missing
<group>once at the top.- Iterates all configured measurements and skips missing logs per measurement with
continueinstead of returning early.- Uses
common_match_casesand increments_countper collected item, matching existing measurement patterns.This also addresses the earlier early‑return and unused‑parameter concerns from the prior review.
Also applies to: 329-345, 377-393
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #828 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3439 3451 +12
=========================================
+ Hits 3439 3451 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.