Use EntityDescription - renault #55061

epenet · 2021-08-23T11:11:30Z

Proposed change

Update renault to use EntityDescription for entity metadata, and add state class for sensors.
-> #53201

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:

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.
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.

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/renault/renault_entities.py

homeassistant/components/renault/binary_sensor.py

homeassistant/components/renault/sensor.py

cdce8p

Just left some last formatting comments.

Aside from that, the overall structure LGTM. The details are a bit more difficult to review, especially since this is such a large PR. I trust that you tested it there.

homeassistant/components/renault/sensor.py

epenet · 2021-08-24T16:00:29Z

Thanks @cdce8p I've fixed the formatting issue on my machine.
I'm just trying to sort out some value_lambda before I ask for a final validation.

epenet · 2021-08-24T16:26:35Z

@cdce8p would you mind taking a final look now that the value_lambda is in?

cdce8p

@epenet The last changes LGTM, too!

epenet · 2021-08-25T05:36:42Z

Great, thanks @cdce8p
Regarding testing - I've done thorough checks on my end, and improved test coverage to include extra attributes (icon, state-class and last-update)
Now the test are failing due to #55169

frenck · 2021-08-25T07:05:07Z

Rebased the PR to deal with the CI issues

epenet · 2021-08-25T09:00:41Z

If it reassures the team, I can split the "test" amends into a preliminary PR to show that functionnality hasn't been impacted at all by the release, and move the "state-class" amends into a subsequent PR afterwards...

cdce8p

LGTM, thanks! Unfortunately we missed the window for 2021.09. Since this is a larger refactoring, I would like to wait until after the release (in a week) before merging it.

Marked the PR as draft to indicate that.

epenet · 2021-08-25T20:57:27Z

aarrgh so close... ;)
I may need to rebase again if #54745 and/or #54820 get merged (to finish the migration from HACS custom_component).
Is it useful for me to get the intermediate "test" amends in a preliminary PR?

cdce8p · 2021-08-25T21:13:36Z

aarrgh so close... ;)
I may need to rebase again if #54745 and/or #54820 get merged (to finish the migration from HACS custom_component).
Is it useful for me to get the intermediate "test" amends in a preliminary PR?

Wasn't aware of that. We had a small discussion on discord how to handle PRs with large refactorings shortly before the release. Turns out I was overly worried 🤷🏻‍♂️ Will merge it now. That should make things easier on your end.

epenet · 2021-08-25T21:23:16Z

Thanks @cdce8p, thanks @frenck

MartinHjelmare · 2021-08-26T00:47:42Z

homeassistant/components/renault/renault_entities.py

+                if self.coordinator.data
+                else None
+            )
+            return {ATTR_LAST_UPDATE: last_update}


We don't include the item at all in the state attributes if the value is None.

Hi @MartinHjelmare you told me the opposite on the original integration: an attribute should never "disappear".
So the way it is implemented is that sensor with battery coordinator will always have the attribute (sometimes with value, and sometimes None) and other coordinators do not return any attributes return None.

Do you have a link to that discussion?

I can't find it. I guess I was mistaken. I'll implement...

MartinHjelmare · 2021-08-26T00:53:12Z

homeassistant/components/renault/sensor.py

+        data_key="chargingStatus",
+        device_class=DEVICE_CLASS_CHARGE_STATE,
+        entity_class=RenaultSensor[KamereonVehicleBatteryStatusData],
+        icon_lambda=lambda x: (


For long expressions it's nicer to define a regular function and assign it here. It doesn't have to be a lambda.

I'll start work on that

MartinHjelmare · 2021-08-26T01:14:52Z

tests/components/renault/test_binary_sensor.py

        state = hass.states.get(entity_id)
        assert state.state == STATE_OFF
+        for attr in CHECK_ATTRIBUTES:
+            if attr == ATTR_LAST_UPDATE:


We want to avoid if/else logic in tests.

Was this OK in sensor.py?
https://github.com/home-assistant/core/blob/dev/tests/components/renault/test_sensor.py#L34-L48

const.py and the fixtures have all the values for when things are OK.
I wanted to avoid having to put in const.py the "error" values.

I don't like that assertion helper. Generally I try to avoid assertion helpers since I think they make it harder to read the test and see what the test is testing. My preference is to have all the setup code including mocking and patching in pytest fixtures but have the function call that is tested and assertions in the test body itself.

But regardless we should keep tests as simple as possible without branching logic as far as possible.

I'll work out a better split between fixed and dynamic attributes

probot-home-assistant bot added code-quality has-tests integration: renault labels Aug 23, 2021

homeassistant added the cla-signed label Aug 23, 2021

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

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

cdce8p reviewed Aug 24, 2021

View reviewed changes

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

cdce8p reviewed Aug 24, 2021

View reviewed changes

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

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

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

cdce8p added the smash Indicator this PR is close to finish for merging or closing label Aug 24, 2021

epenet requested a review from cdce8p August 24, 2021 16:26

cdce8p reviewed Aug 24, 2021

View reviewed changes

epenet added 12 commits August 25, 2021 09:04

Cleanup sensor.py

f55412f

Add EntityDescription

9f327d4

Add checks for state attributes

f23750c

Fix pylint

c8f3427

Simplify checks

fe6c351

Add icon checks

cb9dfad

Update data type

398ac11

Use mixin for required keys, and review class initialisation

0e8b916

Add constraint to TypeVar("T")

0404a11

Enable lambda for icon handling

7fc1981

Enable lambda for value handling

d75e200

Enable lambda for value handling

fcc192b

frenck force-pushed the renault-entity-description branch from 609d20f to fcc192b Compare August 25, 2021 07:04

frenck requested a review from cdce8p August 25, 2021 08:57

cdce8p added project-code_style PRs related to #53201 - pylint CodeStyle update and removed smash Indicator this PR is close to finish for merging or closing labels Aug 25, 2021

cdce8p marked this pull request as draft August 25, 2021 20:49

cdce8p approved these changes Aug 25, 2021

View reviewed changes

cdce8p marked this pull request as ready for review August 25, 2021 21:13

cdce8p merged commit 9315f3b into home-assistant:dev Aug 25, 2021

epenet deleted the renault-entity-description branch August 25, 2021 21:16

MartinHjelmare reviewed Aug 26, 2021

View reviewed changes

epenet mentioned this pull request Aug 26, 2021

Address late review for Renault integration #55230

Merged

22 tasks

github-actions bot locked and limited conversation to collaborators Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use EntityDescription - renault #55061

Use EntityDescription - renault #55061

epenet commented Aug 23, 2021 •

edited

Loading

cdce8p left a comment

epenet commented Aug 24, 2021

epenet commented Aug 24, 2021

cdce8p left a comment

epenet commented Aug 25, 2021

frenck commented Aug 25, 2021

epenet commented Aug 25, 2021

cdce8p left a comment

epenet commented Aug 25, 2021 •

edited

Loading

cdce8p commented Aug 25, 2021

epenet commented Aug 25, 2021

MartinHjelmare Aug 26, 2021

epenet Aug 26, 2021

MartinHjelmare Aug 26, 2021

epenet Aug 26, 2021

MartinHjelmare Aug 26, 2021

epenet Aug 26, 2021

MartinHjelmare Aug 26, 2021

epenet Aug 26, 2021

MartinHjelmare Aug 26, 2021

epenet Aug 26, 2021

Use EntityDescription - renault #55061

Use EntityDescription - renault #55061

Conversation

epenet commented Aug 23, 2021 • edited Loading

Proposed change

Type of change

Additional information

Checklist

cdce8p left a comment

Choose a reason for hiding this comment

epenet commented Aug 24, 2021

epenet commented Aug 24, 2021

cdce8p left a comment

Choose a reason for hiding this comment

epenet commented Aug 25, 2021

frenck commented Aug 25, 2021

epenet commented Aug 25, 2021

cdce8p left a comment

Choose a reason for hiding this comment

epenet commented Aug 25, 2021 • edited Loading

cdce8p commented Aug 25, 2021

epenet commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epenet commented Aug 23, 2021 •

edited

Loading

epenet commented Aug 25, 2021 •

edited

Loading