Enhance schedule handling and cooling #511
Conversation
Reviewer's Guide by SourceryThis pull request enhances schedule handling and cooling functionality in the pyatmo library. It introduces new properties and methods for better management of schedules, improves type hinting, and removes unnecessary comments. The changes also include updates to constants, error handling, and minor refactoring across multiple files. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cgtobi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
src/pyatmo/room.py
Outdated
| @property | ||
| def boiler_status(self) -> bool | None: | ||
| """Return the boiler status.""" | ||
|
|
||
| for module in self.modules.values(): | ||
| if hasattr(module, "boiler_status"): | ||
| if (boiler_status := module.boiler_status) is not None: | ||
| return boiler_status | ||
|
|
||
| return None | ||
|
|
||
| @property | ||
| def setpoint_mode(self) -> str: | ||
| """Return the current setpoint mode.""" | ||
|
|
||
| return self.therm_setpoint_mode or self.cooling_setpoint_mode or UNKNOWN | ||
|
|
||
| @property | ||
| def setpoint_temperature(self) -> float | None: | ||
| """Return the current setpoint temperature.""" | ||
|
|
||
| return ( | ||
| self.therm_setpoint_temperature or self.cooling_setpoint_temperature or None | ||
| ) | ||
|
|
||
| @property | ||
| def hvac_action(self) -> str: | ||
| """Return the current HVAC action.""" | ||
|
|
||
| if self.setpoint_mode == OFF: | ||
| return OFF | ||
|
|
||
| if self.climate_type != DeviceType.NRV and self.boiler_status is not None: | ||
| return HEATING if self.boiler_status else IDLE | ||
|
|
||
| if self.boiler_status is True: | ||
| return HEATING | ||
|
|
||
| if self.heating_power_request is not None and self.heating_power_request > 0: | ||
| return HEATING | ||
|
|
||
| if self.heating_power_request: | ||
| return HEATING | ||
| if self.cooling_setpoint_temperature: | ||
| return COOLING | ||
|
|
||
| return IDLE |
There was a problem hiding this comment.
suggestion: Add docstrings to new HVAC-related methods
Please add docstrings to the new methods (boiler_status, setpoint_mode, setpoint_temperature, hvac_action) explaining their purpose, return values, and any important logic they contain. This will greatly improve the code's readability and maintainability.
@property
def boiler_status(self) -> bool | None:
"""
Return the boiler status.
Iterates through modules to find and return the first non-None boiler status.
Returns None if no boiler status is found.
"""
@property
def setpoint_mode(self) -> str:
"""
Return the current setpoint mode.
Prioritizes therm_setpoint_mode, then cooling_setpoint_mode.
Returns UNKNOWN if neither is set.
"""
@property
def setpoint_temperature(self) -> float | None:
"""
Return the current setpoint temperature.
Prioritizes therm_setpoint_temperature, then cooling_setpoint_temperature.
Returns None if neither is set.
"""
@property
def hvac_action(self) -> str:
"""
Return the current HVAC action.
Determines action based on setpoint mode, boiler status, and power requests.
Returns OFF, HEATING, COOLING, or IDLE based on current state.
"""
| for schedule in self.schedules.values() | ||
| if self.temperature_control_mode | ||
| and schedule.type == SCHEDULE_TYPE_MAPPING[self.temperature_control_mode] | ||
| ] |
There was a problem hiding this comment.
suggestion: Add inline comments to explain complex schedule handling logic
The logic in get_selected_schedule and get_available_schedules is quite dense. Consider adding some inline comments to explain the reasoning behind the conditions in the list comprehensions. This will make the code easier to understand and maintain.
| @@ -27,7 +27,7 @@ async def main(): | |||
|
|
|||
| await account.async_update_status(home_id=home_id) | |||
|
|
|||
| strt = 1709766000 + 10 * 60 # 1709421000+15*60 | |||
| strt = 1709766000 + 10 * 60 | |||
There was a problem hiding this comment.
suggestion (testing): Consider using datetime objects for improved readability and maintainability
Instead of using raw timestamps, consider using datetime objects and timedelta for better readability and easier maintenance. For example: strt = datetime(2024, 3, 6, 12, 10).timestamp()
from datetime import datetime, timedelta
strt = (datetime(2024, 3, 6, 12, 0) + timedelta(minutes=10)).timestamp()
| @@ -27,7 +27,7 @@ async def main(): | |||
|
|
|||
| await account.async_update_status(home_id=home_id) | |||
|
|
|||
| strt = 1709766000 + 10 * 60 # 1709421000+15*60 | |||
| strt = 1709766000 + 10 * 60 | |||
| end = 1709852400 + 10 * 60 | |||
There was a problem hiding this comment.
suggestion (testing): Use consistent naming conventions for variables
Consider renaming 'strt' to 'start' for consistency with the 'end' variable and to improve code readability.
| if self.cooling_setpoint_temperature: | ||
| return COOLING | ||
|
|
||
| return IDLE |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp)
| if self.cooling_setpoint_temperature: | |
| return COOLING | |
| return IDLE | |
| return COOLING if self.cooling_setpoint_temperature else IDLE |
cgtobi
force-pushed
the
cooling
branch
2 times, most recently
from
d1f1ad0 to
edf70b5
Compare
Summary by Sourcery
Enhance the schedule handling and cooling features by introducing new properties and methods for managing boiler status, setpoint modes, and HVAC actions. Implement a new ScheduleType enum and improve schedule selection logic. Refactor code for better readability and update pre-commit configuration.
New Features:
Enhancements:
Build:
Tests: