Add `state_class` and use `SensorEntityDescription` for comfoconnect by michaelarnauts · Pull Request #54066 · home-assistant/core

michaelarnauts · 2021-08-05T13:55:33Z

Proposed change

Add state_class so the power stats work with the new energy dashboard.
Use native_value and native_unit_of_measurement
Refactor to use SensorEntityDescriptions for templates in this integration.

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)
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: Update Comfoconnect documentation home-assistant.io#19191

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
The code has been formatted using Black (black --fast 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.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

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

homeassistant/components/comfoconnect/sensor.py

homeassistant/components/comfoconnect/fan.py

homeassistant/components/comfoconnect/sensor.py

homeassistant/components/comfoconnect/fan.py

homeassistant/components/comfoconnect/sensor.py

homeassistant/components/comfoconnect/fan.py

michaelarnauts · 2021-08-11T19:41:46Z

I have a conflict now since #48261 has been merged. Should I use native_unit_of_measurement and _attr_native_value now for all sensors?

michaelarnauts · 2021-08-11T19:44:14Z

I also had to modify homeassistant/components/fan/__init__.py (https://github.com/home-assistant/core/pull/54066/files#diff-f432c8111ba270e7e035c3f131c7dcd5ff1398fdc740eaaa65e64873f13ecc55) since it didn't support the _attr_supported_features yet. This then caused issues in state_attributes since the typing is different.

MartinHjelmare · 2021-08-12T05:58:55Z

homeassistant/components/comfoconnect/sensor.py

    {
        vol.Optional(CONF_RESOURCES, default=[]): vol.All(
-            cv.ensure_list, [vol.In(SENSOR_TYPES)]
+            cv.ensure_list, [vol.In([desc.key for desc in SENSOR_TYPES])]


Side note: Long term we should remove this option and just allow users to enable/disable entities as wanted via the entity registry, and we set the less important entities as disabled by default. That means the description key will change to be the sensor_id.

I want to take a look at converting this to a config flow later, and then this will go away anyway.

homeassistant/components/comfoconnect/sensor.py

michaelarnauts · 2021-08-18T13:43:10Z

The required changes have been made, and it's up to date with the latest dev branch.

Is it okay for the small change in homeassistant/components/fan/__init__.py to go in here? Or should that happen in a separate PR?

homeassistant/components/comfoconnect/fan.py

homeassistant/components/fan/__init__.py

michaelarnauts · 2021-09-03T08:59:10Z

The changes in fan have been reverted.

MartinHjelmare · 2021-09-03T12:48:52Z

There's a test failure.

michaelarnauts · 2021-09-03T12:58:16Z

There's a test failure.

Sorry. I'm looking into it.

michaelarnauts · 2021-09-03T15:12:08Z

Sorry for the annoying commits. I'm still waiting for tox to fully install all dependencies so I can run the tests locally. This seems to take ages. :(

cdce8p · 2021-09-03T15:32:04Z

@michaelarnauts You don't necessarily need tox for that. As long as the dependancies are installed locally, invoking pytest directly will also work. That's the setup I tend to use: venv with all basic requirements + those for the component I want to run the tests for. Than pytest tests/components/xxx/

That might be helpful as well: https://developers.home-assistant.io/docs/development_testing#testing-outside-of-tox

michaelarnauts · 2021-09-03T16:25:48Z

@michaelarnauts You don't necessarily need tox for that. As long as the dependancies are installed locally, invoking pytest directly will also work. That's the setup I tend to use: venv with all basic requirements + those for the component I want to run the tests for. Than pytest tests/components/xxx/

That might be helpful as well: https://developers.home-assistant.io/docs/development_testing#testing-outside-of-tox

I tried that, but it complained about missing pool fixtures or something like that. I haven't had the time do fully dive into how home assistant runs its tests.

homeassistant/components/fan/__init__.py

homeassistant/components/comfoconnect/sensor.py

michaelarnauts · 2021-09-08T10:27:35Z

Reverted attribute key changes, and removed breaking change from ticket.

homeassistant/components/comfoconnect/fan.py

michaelarnauts · 2021-09-23T09:12:46Z

All done!

MartinHjelmare

Thanks!

probot-home-assistant bot added code-quality integration: comfoconnect labels Aug 5, 2021

homeassistant added the cla-signed label Aug 5, 2021

probot-home-assistant bot added the by-code-owner label Aug 5, 2021

Add state_class=measurement and use SensorEntityDescriptions

45a3bd6

michaelarnauts force-pushed the comfoconnect_state_class branch from 9ea534a to 45a3bd6 Compare August 5, 2021 13:56

ctalkington requested changes Aug 6, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Show resolved Hide resolved

Use attributes from entity_description

26d50c3

frenck added energy Related to energy management smash Indicator this PR is close to finish for merging or closing labels Aug 9, 2021

balloob reviewed Aug 10, 2021

View reviewed changes

homeassistant/components/comfoconnect/fan.py Show resolved Hide resolved

balloob reviewed Aug 10, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Show resolved Hide resolved

balloob reviewed Aug 10, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Show resolved Hide resolved

balloob reviewed Aug 10, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Outdated Show resolved Hide resolved

balloob reviewed Aug 10, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Outdated Show resolved Hide resolved

cdce8p reviewed Aug 10, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Outdated Show resolved Hide resolved

homeassistant/components/comfoconnect/sensor.py Outdated Show resolved Hide resolved

Improvements

8470f9e

ludeeus reviewed Aug 11, 2021

View reviewed changes

frenck reviewed Aug 11, 2021

View reviewed changes

homeassistant/components/comfoconnect/fan.py Outdated Show resolved Hide resolved

homeassistant/components/comfoconnect/fan.py Outdated Show resolved Hide resolved

Adress remarks

8d5fd6e

MartinHjelmare reviewed Aug 12, 2021

View reviewed changes

ludeeus reviewed Aug 12, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Outdated Show resolved Hide resolved

homeassistant/components/comfoconnect/sensor.py Outdated Show resolved Hide resolved

Merge branch 'dev' into comfoconnect_state_class

ceff100

frenck requested changes Aug 21, 2021

View reviewed changes

homeassistant/components/comfoconnect/fan.py Outdated Show resolved Hide resolved

frenck reviewed Aug 21, 2021

View reviewed changes

homeassistant/components/fan/__init__.py Show resolved Hide resolved

frenck removed the smash Indicator this PR is close to finish for merging or closing label Aug 25, 2021

Revert changes to fan

21aaf36

move method

efb9dbe

michaelarnauts requested a review from frenck September 3, 2021 09:00

michaelarnauts mentioned this pull request Sep 3, 2021

Add additional comfoconnect settings #55652

Closed

22 tasks

michaelarnauts requested a review from ctalkington September 3, 2021 09:39

michaelarnauts mentioned this pull request Sep 3, 2021

Update Comfoconnect documentation home-assistant/home-assistant.io#19191

Closed

8 tasks

Fix tests

2c523b1

michaelarnauts force-pushed the comfoconnect_state_class branch from e148ff5 to 2c523b1 Compare September 3, 2021 15:19

cdce8p added the project-code_style PRs related to #53201 - pylint CodeStyle update label Sep 4, 2021

MartinHjelmare reviewed Sep 4, 2021

View reviewed changes

homeassistant/components/fan/__init__.py Outdated Show resolved Hide resolved

MartinHjelmare reviewed Sep 5, 2021

View reviewed changes

homeassistant/components/comfoconnect/sensor.py Outdated Show resolved Hide resolved

michaelarnauts added 2 commits September 8, 2021 10:04

Revert fan/__init__.py

06c504b

Revert key change

e015718

michaelarnauts requested a review from MartinHjelmare September 10, 2021 08:12

MartinHjelmare reviewed Sep 15, 2021

View reviewed changes

homeassistant/components/comfoconnect/fan.py Outdated Show resolved Hide resolved

Set default percentage in turn_on

88f6691

MartinHjelmare approved these changes Sep 24, 2021

View reviewed changes

MartinHjelmare merged commit 5d3d6fa into home-assistant:dev Sep 24, 2021

github-actions bot locked and limited conversation to collaborators Sep 25, 2021

Uh oh!

Conversation

michaelarnauts commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelarnauts commented Aug 11, 2021

Uh oh!

michaelarnauts commented Aug 11, 2021

Uh oh!

MartinHjelmare Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

michaelarnauts Aug 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelarnauts commented Aug 18, 2021

Uh oh!

Uh oh!

Uh oh!

michaelarnauts commented Sep 3, 2021

Uh oh!

MartinHjelmare commented Sep 3, 2021

Uh oh!

michaelarnauts commented Sep 3, 2021

Uh oh!

michaelarnauts commented Sep 3, 2021

Uh oh!

cdce8p commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelarnauts commented Sep 3, 2021

Uh oh!

Uh oh!

Uh oh!

michaelarnauts commented Sep 8, 2021

Uh oh!

Uh oh!

michaelarnauts commented Sep 23, 2021

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

michaelarnauts commented Aug 5, 2021 •

edited

Loading

cdce8p commented Sep 3, 2021 •

edited

Loading