Tado code quality improvements by erwindouna · Pull Request #107678 · home-assistant/core

erwindouna · 2024-01-09T18:49:43Z

Breaking change

Proposed change

Refactor the files Binary Sensor, Climate and Water Heater to be more strict and add typing where missing. Also some various code improvements have been made to make code less redundant and fit with typing.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2024-01-09T18:49:51Z

Hey there @michaelarnauts, @chiefdragon, mind taking a look at this pull request as it has been labeled with an integration (tado) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of tado can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign tado Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

edenhaus

Please update the description, as there are changes outside the binary sensor.
It looks like there are also some bug fixes. Please add them to the description, too

edenhaus · 2024-01-10T16:23:39Z

        self._attr_fan_modes = supported_fan_modes
        self._attr_supported_features = support_flags

+        # Why are they here? Part of generic thermostat it seems? Need to be set?


Why is this question here?

They seem unused. Is needed for all climate controls? I simply don't know why it's there at this point.

Then please remove them if they are not used in the parent classes.

edenhaus · 2024-01-10T16:29:20Z


-    def _normalize_target_temp_for_hvac_mode(self):
+    def _normalize_target_temp_for_hvac_mode(self) -> None:
+        def adjust_temp(min_temp, max_temp):


Why is this an inline function?
It should be a util function IMHO

The only use case is within this function. That's why I put it inline.

Pleas ad typing to it

home-assistant · 2024-01-10T16:34:17Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Co-authored-by: Robert Resch <robert@resch.dev>

edenhaus · 2024-01-24T14:10:58Z

        self._attr_fan_modes = supported_fan_modes
        self._attr_supported_features = support_flags

+        # Why are they here? Part of generic thermostat it seems? Need to be set?


Then please remove them if they are not used in the parent classes.

edenhaus · 2024-01-24T14:17:05Z


-    def _normalize_target_temp_for_hvac_mode(self):
+    def _normalize_target_temp_for_hvac_mode(self) -> None:
+        def adjust_temp(min_temp, max_temp):


Pleas ad typing to it

edenhaus

If the integration is completely typed we can add tado to .strict-typing and add a section for it in mypy.ini (If not already done)

Co-authored-by: Robert Resch <robert@resch.dev>

erwindouna · 2024-02-27T14:17:25Z

If the integration is completely typed we can add tado to .strict-typing and add a section for it in mypy.ini (If not already done)

This would also require the __init__.py. This is a rather large one. May I suggest to do that in a separate PR and then add it to .strict-typing?

edenhaus

Thanks @erwindouna 👍

Code quality improvements to Binary Sensor

5faebc3

home-assistant Bot added cla-signed integration: tado small-pr PRs with less than 30 lines. labels Jan 9, 2024

home-assistant Bot assigned michaelarnauts Jan 9, 2024

home-assistant Bot added by-code-owner Quality Scale: No score labels Jan 9, 2024

erwindouna changed the title ~~Code quality improvements to Binary Sensor~~ Tado code quality improvements to Binary Sensor Jan 9, 2024

erwindouna marked this pull request as ready for review January 9, 2024 18:50

erwindouna requested a review from michaelarnauts as a code owner January 9, 2024 18:50

erwindouna added 5 commits January 9, 2024 20:23

Adding Climate

9aa72ae

Forgot Geofence

1084abf

Refactor constructor

4b541c4

Adding water heater

2cccc27

Question about _cur_temp

7f688b2

erwindouna changed the title ~~Tado code quality improvements to Binary Sensor~~ Tado code quality improvements to Binary Sensor, Climate and Water Heater Jan 9, 2024

michaelarnauts removed their assignment Jan 10, 2024

zweckj reviewed Jan 10, 2024

View reviewed changes

Comment thread homeassistant/components/tado/climate.py Outdated

Comment thread homeassistant/components/tado/climate.py Outdated

Refactor code to check float instance

76b7d5c

zweckj reviewed Jan 10, 2024

View reviewed changes

Comment thread homeassistant/components/tado/climate.py Outdated

Optimize code

875f386

edenhaus requested changes Jan 10, 2024

View reviewed changes

home-assistant Bot marked this pull request as draft January 10, 2024 16:34

erwindouna and others added 2 commits January 10, 2024 18:42

Update homeassistant/components/tado/water_heater.py

7d348aa

Co-authored-by: Robert Resch <robert@resch.dev>

Feedback fixes

35b93ac

erwindouna requested a review from edenhaus January 10, 2024 17:48

erwindouna marked this pull request as ready for review January 10, 2024 17:48

edenhaus requested changes Jan 24, 2024

View reviewed changes

home-assistant Bot marked this pull request as draft January 24, 2024 14:19

erwindouna added 3 commits February 25, 2024 18:29

Removing comment

5d6d17d

Patching feedback

683a7cc

Removing last optionals

ad0e595

erwindouna marked this pull request as ready for review February 25, 2024 18:39

home-assistant Bot requested a review from edenhaus February 25, 2024 18:39

edenhaus requested changes Feb 27, 2024

View reviewed changes

Comment thread homeassistant/components/tado/water_heater.py Outdated

Comment thread homeassistant/components/tado/water_heater.py Outdated

Comment thread homeassistant/components/tado/water_heater.py Outdated

Comment thread homeassistant/components/tado/water_heater.py Outdated

home-assistant Bot marked this pull request as draft February 27, 2024 13:02

erwindouna and others added 8 commits February 27, 2024 14:14

Update homeassistant/components/tado/water_heater.py

5e544b0

Co-authored-by: Robert Resch <robert@resch.dev>

Update homeassistant/components/tado/water_heater.py

d04146c

Co-authored-by: Robert Resch <robert@resch.dev>

Update homeassistant/components/tado/water_heater.py

85ec561

Co-authored-by: Robert Resch <robert@resch.dev>

Update homeassistant/components/tado/water_heater.py

032be4a

Co-authored-by: Robert Resch <robert@resch.dev>

Add to strict-typing and mypy files

d48a097

Merge branch 'dev' into tado-binary-sensor-typing

f80ec58

Merge branch 'dev' into tado-binary-sensor-typing

7352530

Reverting back

f9099a5

Fix import

375f117

erwindouna marked this pull request as ready for review February 27, 2024 14:37

home-assistant Bot requested a review from edenhaus February 27, 2024 14:37

edenhaus approved these changes Feb 28, 2024

View reviewed changes

edenhaus changed the title ~~Tado code quality improvements to Binary Sensor, Climate and Water Heater~~ Tado code quality improvements Feb 28, 2024

edenhaus merged commit c52e203 into home-assistant:dev Feb 28, 2024

erwindouna deleted the tado-binary-sensor-typing branch February 28, 2024 11:04

github-actions Bot locked and limited conversation to collaborators Feb 29, 2024

Uh oh!

Conversation

erwindouna commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

home-assistant Bot commented Jan 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edenhaus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

home-assistant Bot commented Jan 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edenhaus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erwindouna commented Feb 27, 2024

Uh oh!

edenhaus left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erwindouna commented Jan 9, 2024 •

edited

Loading