Implement improvement-contributions from digisaster #713
Conversation
|
Warning Rate limit exceeded@bouwew has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 12 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 (2)
WalkthroughThe changes introduce a new constant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise/util.py (1)
235-235: LGTM! Consider documenting the side effect.The function logic has been improved by:
- Using
getwith a default value for safer dictionary access- Conditionally rounding only when
f_valis a float- Modifying
datain place instead of returning itConsider adding a docstring note about the side effect of modifying the
dataparameter in place:def power_data_energy_diff( measurement: str, net_string: SensorType, f_val: float | int, data: GwEntityData, ) -> None: - """Calculate differential energy.""" + """Calculate differential energy. + + Note: This function modifies the `data` parameter in place. + """Also applies to: 242-246
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise/util.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run pytest using Python 3.12
- GitHub Check: Run pytest using Python 3.13.0
🔇 Additional comments (3)
plugwise/util.py (3)
31-31: LGTM!The import of
DataMissingErroris correctly placed and used in thecheck_heater_centralfunction.
97-100: LGTM! Consider adding test coverage for the error case.The error handling for missing central heating boiler is well-implemented with a descriptive message. However, the
pragma: no coversuggests this edge case isn't tested.Would you like me to help generate test cases for this error scenario?
209-215: LGTM!The rounding logic has been improved with clearer, more direct assignments while maintaining consistent precision levels.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #713 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3419 3418 -1
=========================================
- Hits 3419 3418 -1 ☔ View full report in Codecov by Sentry. |
plugwise/util.py
Outdated
| if not hc_list: | ||
| raise DataMissingError( | ||
| "No Central heating boiler found, please create an Issue" | ||
| ) # pragma: no cover |
There was a problem hiding this comment.
Thats a bit vague - as we probably re-raise this from the front-end, I'd suggest we make a BoilerDataMissingError (and don't care too much about the actual message since we don't necessarily raise user errors from the supporting lib
There was a problem hiding this comment.
Agreed, I'll let it return NONE then we can detect that someplace else if we really want that.
|
In a next PR, checking for the presence of a |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise/util.py (1)
32-32: Consider implementing error handling usingDataMissingError.The
DataMissingErroris imported but never used. Based on past review comments, this exception was intended to be used for handling missing boiler data. Consider implementing proper error handling:
- Raise
DataMissingErrorincheck_heater_centralwhenhc_listis empty- Handle the exception in the frontend code
Apply this diff to implement error handling:
def check_heater_central(xml: etree.Element) -> 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']" hc_count = 0 hc_list: list[dict[str, bool]] = [] for heater_central in xml.findall(locator): hc_count += 1 hc_id: str = heater_central.attrib["id"] has_actuators: bool = ( heater_central.find("actuator_functionalities/") is not None ) # Filter for Plug/Circle/Stealth heater_central -- Pw-Beta Issue #739 if heater_central.find("name").text == "Central heating boiler": hc_list.append({hc_id: has_actuators}) if not hc_list: - return NONE # pragma: no cover + raise DataMissingError("No valid heater central found") # pragma: no cover🧰 Tools
🪛 Ruff (0.8.2)
32-32:
plugwise.exceptions.DataMissingErrorimported but unusedRemove unused import:
plugwise.exceptions.DataMissingError(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.py(1 hunks)plugwise/util.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
plugwise/util.py
32-32: plugwise.exceptions.DataMissingError imported but unused
Remove unused import: plugwise.exceptions.DataMissingError
(F401)
🔇 Additional comments (4)
plugwise/common.py (1)
79-81: LGTM! Early return pattern improves error handling.The addition of an early return when no valid heater ID is found prevents unnecessary processing and improves the control flow.
plugwise/util.py (3)
98-100: LGTM! Improved error handling for empty heater list.The addition of an early return with
NONEwhen no heater is found improves error handling and aligns with the changes in_appl_heater_central_info.
207-214: LGTM! Streamlined rounding logic.The rounding logic has been simplified and optimized while maintaining the same functionality.
240-245: LGTM! Simplified differential energy calculation.The logic for calculating differential energy has been streamlined and improved.
bouwew
force-pushed
the
digisaster-contrib
branch
from
a842e36 to
ad09f6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugwise/util.py (2)
98-100: Add test coverage for emptyhc_listcase.The empty list check is marked with
# pragma: no cover, indicating missing test coverage for this edge case.Would you like me to help generate test cases to cover this scenario?
104-107: Consider improving readability of heater ID selection.While using
next(iter())is more concise, it might be less readable. Consider using a more explicit approach:- if next(iter(item.values())): - heater_central_id = next(iter(item)) # pragma: no cover - # Stop when a valid id is found - break # pragma: no cover + heater_id, has_actuators = next(iter(item.items())) # Unpack the only key-value pair + if has_actuators: + heater_central_id = heater_id # pragma: no cover + break # Stop when a valid id is found # pragma: no cover
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.py(1 hunks)plugwise/util.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise/common.py
🧰 Additional context used
🪛 Ruff (0.8.2)
plugwise/util.py
32-32: plugwise.exceptions.DataMissingError imported but unused
Remove unused import: plugwise.exceptions.DataMissingError
(F401)
🪛 GitHub Actions: Latest commit
plugwise/util.py
[error] 32-32: Ruff: plugwise.exceptions.DataMissingError imported but unused. Remove unused import.
🔇 Additional comments (2)
plugwise/util.py (2)
233-245: LGTM! Good improvements to the energy calculation logic.The changes improve the function by:
- Modifying data in place instead of returning it
- Adding type safety for rounding operations
- Simplifying the differential energy calculation
207-214: LGTM! Clear and well-structured rounding logic.The changes improve readability by making the rounding rules explicit and easier to understand.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise/common.py(1 hunks)plugwise/util.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise/common.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Create pytest cache for Python 3.12
- GitHub Check: Create pytest cache for Python 3.13.0
🔇 Additional comments (5)
plugwise/util.py (5)
16-16: LGTM!The
NONEconstant import is correctly placed with other constants fromplugwise.constants.
97-98: LGTM!Good defensive programming practice to handle the case when no valid heater central is found.
206-213: LGTM!The rounding logic has been simplified and made more consistent:
- 3 decimal places for special formats
- 1 decimal place for voltage measurements
- 2 decimal places for values < 10
- 1 decimal place for values >= 10
232-244: LGTM!Good improvements to the function:
- Changed to modify data in-place instead of returning it
- Simplified energy differential calculation
- Added safe dictionary access using
get- Made rounding conditional on value type
141-141: LGTM!The removal of data assignment aligns with
power_data_energy_diffnow modifying data in-place.
bouwew
force-pushed
the
digisaster-contrib
branch
from
29871fd to
d90fe51
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise/util.py (1)
103-106: Consider improving readability and test coverage.The logic simplification using
next(iter())is more concise but might be less intuitive. Additionally, these code paths are not covered by tests.Consider this more readable alternative:
- if next(iter(item.values())): - heater_central_id = next(iter(item)) # pragma: no cover - # Stop when a valid id is found - break # pragma: no cover + hc_id, has_actuators = next(iter(item.items())) + if has_actuators: + heater_central_id = hc_id + breakAlso, please add test cases for multiple heater central scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise/common.py(1 hunks)plugwise/helper.py(1 hunks)plugwise/util.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise/common.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check commit
🔇 Additional comments (6)
plugwise/helper.py (1)
256-256: LGTM! Improved safety by returning a new Munch instance.The change ensures that unrecognized appliance types return a clean, empty object instead of potentially returning a modified input object. This is a safer approach that prevents data leakage and makes the behavior more predictable.
plugwise/util.py (5)
16-16: LGTM!Good practice to import and use a constant instead of a magic value.
97-98: LGTM!Good addition of an early return when no heater central is found, improving code readability.
141-141: LGTM!Correctly removed assignment as
power_data_energy_diffnow modifies the data in place.
206-213: LGTM!The rounding logic is now more consistent and easier to follow with clear conditions for different precisions.
232-244: LGTM!Good improvements:
- Return type now correctly indicates the function modifies data in place
- More concise and clearer logic for energy differential calculation
- Conditional rounding based on the value type
bouwew
force-pushed
the
digisaster-contrib
branch
from
d90fe51 to
e814e3d
Compare
|
bouwew
changed the title
And adapt/improve where required/possible
Summary by CodeRabbit
New Features
NONEfor better error handling.Refactor