Add options to SelectEntityDescription #78882

epenet · 2022-09-21T11:00:04Z

Breaking change

FOR CUSTOM COMPONENTS ONLY:

As of Home Assistant Core 2022.11, options is available as a standard property of SelectEntityDescription.
This may cause issues in custom components if a custom options property was previously implemented.

Please adjust the custom component by either dropping or renaming the custom options property.

Proposed change

Add options to SelectEntityDescription

I was originally surprised that options was not part of the standard SelectEntityDescription and I thought it was missed in the initial implementation.
Then I bumped into the issue that "we don't want a default" so I backed out.
Then I received the request from @MartinHjelmare in #78873 (comment) so I worked on it again and found a (maybe not pretty) solution.

Looking at the SelectEntityDescription in the core, it looks like more components would also benefit:

kostal_plenticore, overkiz, renault and xiaomi_miio use a custom options property that is updated in this PR
goodwe, lifx and senseme use a local constant which could be moved to the SelectEntityDescription
litterrobot, roku, sensibo, tuya and unifiprotect use a dynamic list, and would not be impacted by this PR
plugwise uses options in a weird way, adjusted in Rename property in Plugwise EntityDescription #78935
rainmachine uses options in a weird way, adjusted in Rename options key in rainmachine #79249

So 7 out of 14 components would directly benefit from this, if it is accepted.

HACS:

These use a custom options property that replicates this PR

LavermanJJ/home-assistant-solarfocus Options is available as a standard property of SelectEntityDescription LavermanJJ/home-assistant-solarfocus#13
elad-bar/ha-shinobi Options is available as a standard property of SelectEntityDescription elad-bar/ha-shinobi#35
iMicknl/ha-nest-protect Options is available as a standard property of SelectEntityDescription iMicknl/ha-nest-protect#106
imicknl/ha-tahoma Options is available as a standard property of SelectEntityDescription iMicknl/ha-tahoma#820
jcgoette/baby_buddy_homeassistant: Options is available as a standard property of SelectEntityDescription jcgoette/baby_buddy_homeassistant#80
syssi/homeassistant-goecharger-mqtt: Options is available as a standard property of SelectEntityDescription syssi/homeassistant-goecharger-mqtt#52
wills106/homsassistant-solax-modbus: Options is available as a standard property of SelectEntityDescription wills106/homeassistant-solax-modbus#140

These do not use a custom options property and are not impacted by the change

DeebotUniverse/Deebot-4-Home-Assistant
MTrab/landroid_cloud
dmamontov/hass-ledfx
dmamontov/hass-miwifi
eifinger/hass-foldingathomecontrol
eifinger/hass-weenect
mletenay/home-assistant-goodwe-inverter-
music-assistant/hass-music-assistant
wlcrs/huawei_solar

If it is not accepted, then I am happy to close (and maybe adjust the seven components to use the same code style).

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: Add blog post for SelectEntityDescription developers.home-assistant#1493

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.

probot-home-assistant · 2022-09-21T11:00:11Z

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (select) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

homeassistant/components/select/__init__.py

MartinHjelmare · 2022-09-21T11:53:23Z

homeassistant/components/select/__init__.py

@@ -72,6 +72,8 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
 class SelectEntityDescription(EntityDescription):
    """A class that describes select entities."""

+    options: list[str] | UndefinedType = UNDEFINED


Can we skip the UndefinedType to make it easier with type checking in all the platforms that use this description attribute? UNDEFINED is just needed here in the integration to make it optional with a default.

Done, using type: ignore

epenet · 2022-09-21T11:43:15Z

homeassistant/components/plugwise/select.py

@@ -25,7 +25,7 @@ class PlugwiseSelectDescriptionMixin:

    command: Callable[[Smile, str, str], Awaitable[Any]]
    current_option: str
-    options: str
+    options_key: str


There is a risk of breaking change for components that have the options key already defined.

We should perhaps make a dev blog, and search HACS for matches?

homeassistant/components/select/__init__.py

epenet · 2022-09-21T12:13:35Z

homeassistant/components/select/__init__.py

@@ -72,6 +72,8 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
 class SelectEntityDescription(EntityDescription):
    """A class that describes select entities."""

+    options: list[str] | UndefinedType = UNDEFINED


Done, using type: ignore

MartinHjelmare

@frenck what do you think?

frenck · 2022-09-21T12:59:17Z

homeassistant/components/select/__init__.py

@@ -72,6 +72,8 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
 class SelectEntityDescription(EntityDescription):
    """A class that describes select entities."""

+    options: list[str] = UNDEFINED  # type: ignore[assignment]


This feels way too hacky for my taste (messing with the typing that is).

It's convenience. Same reason why we ignore type for hass in Entity.

What we do in hass is a very exceptional case.
In this case, options cannot be None, so we don't need to use sentinel values.

As in: We don't need to "hacky" around it or use typing exceptions to make this work nicely.

Suggested change

options: list[str] = UNDEFINED # type: ignore[assignment]

options: list[str] | None = None

EDIT: this comment looks weird, as its shown as part of a review, there is more context above. This was original a response to Martin.

frenck · 2022-09-21T13:02:08Z

I'm kinda lost on why we do this? (Motivation? SSIA doesn't help in this case).
As in, EntityDescription are pretty much limited to their basic, descriptive, properties, not to their options/data.

Adding this level of things to the entity descriptions by default, opens up a can of worms we originally avoided. For example, effects in lights, HVAC modes and many others.

It could be fine to implement those as well, but I think it should be consistent if we do that (and not just one off).

epenet · 2022-09-21T13:04:40Z

I'm kinda lost on why we do this? (Motivation? SSIA doesn't help in this case).

I have adjusted the PR description. It originated from #78873 (comment)

MartinHjelmare · 2022-09-21T13:18:12Z

We already use the descriptions for many different kind of attributes in the implementing integrations. I don't think the can of worms is closed.

frenck · 2022-09-21T13:27:58Z

We already use the descriptions for many different kind of attributes in the implementing integrations.

Yeah sure, they can extend it they way they like.

epenet · 2022-09-21T15:22:51Z

I was surprised that options was not part of the standard SelectEntityDescription and I thought it was missed in the initial implementation.
Then I bumped into the issue that "we don't want a default" so I backed out.
Then I received the request from @MartinHjelmare so I worked on it again and found a (maybe not pretty) solution.

Looking at the SelectEntityDescription in the core, it looks like more components would also benefit:

kostal_plenticore, overkiz, renault and xiaomi_miio use a custom options property that it updated in this PR
goodwe, lifx and senseme use a local constant which could be moved to the SelectEntityDescription
litterrobot, roku, sensibo, tuya and unifiprotect use a dynamic list, and would not be impacted by this PR
plugwise uses options in a weird way

So 7 out of 13 components would directly benefit from this, if it is accepted.

If it is not accepted, then I am happy to close (and maybe adjust the seven components to use the same code style).

bouwew · 2022-09-22T08:33:58Z

@epenet for plugwise, I would suggest changing current_option: str to current_option_key: str as well. And the occurrences further down as well.
current_option is used as a key, like the updated option_key.

In plugwise number.py the use of the addition _key was already implemented. Select.py was on the todo-list, but since you're already updating things, let's update all :)

epenet · 2022-09-22T09:03:21Z

@epenet for plugwise, I would suggest changing current_option: str to current_option_key: str as well. And the occurrences further down as well. current_option is used as a key, like the updated option_key.

In plugwise number.py the use of the addition _key was already implemented. Select.py was on the todo-list, but since you're already updating things, let's update all :)

I am not sure if this PR will be accepted or not. I think plugwise should be adjusted regardless to avoid confusion and I have opened the corresponding PR: #78935

epenet · 2022-09-28T18:35:02Z

There was a conflict... solved now.

frenck · 2022-09-28T18:35:45Z

👍 Will be getting back to this after the beta stuff is out.

epenet · 2022-09-28T18:57:13Z

rainmachine has crept in - I'll have to look at that tomorrow

epenet · 2022-10-10T07:47:53Z

I have rebased over dev to pick-up lametric which also benefits from this change.

frenck · 2022-10-10T07:54:01Z

That is the reason why I didn't implement current_option which gets adjusted throughout the life of the entity.

Sure, but it can provide a default. When setting the entity description options, it still leaves room for adjusting it during the lifetime, for example, by setting self._attr_options during an update.

epenet · 2022-10-10T08:07:33Z

That is the reason why I didn't implement current_option which gets adjusted throughout the life of the entity.

Sure, but it can provide a default. When setting the entity description options, it still leaves room for adjusting it during the lifetime, for example, by setting self._attr_options during an update.

I have added it because it was requested, but I couldn't find a use case for it in the core.

frenck · 2022-10-10T08:07:51Z

I've seen you've applied current_option already, but I would love to hear the opinion of @MartinHjelmare on this as well.

I have added it because it was requested, but I couldn't find a use case for it in the core.

Well, that might be a sign not to do it 🤷
That said, select isn't a widely used platform. The default state of, e.g., switch or binary_sensors are more common maybe.

This reverts commit 296ace2.

MartinHjelmare · 2022-10-10T10:05:38Z

I think it's good to be consistent and have attributes for all properties in the descriptions but if we don't see a use case in the current code we could wait with adding it.

epenet · 2022-10-10T10:09:30Z

I think it's good to be consistent and have attributes for all properties in the descriptions but if we don't see a use case in the current code we could wait with adding it.

I agree that all properties in the description should have corresponding attributes, and I think that's already the case.
Conversely however, I think only static attributes should have corresponding properties in the description.

frenck · 2022-10-10T10:20:08Z

Conversely however, I think only static attributes should have corresponding properties in the description.

In that case, this PR is invalid, as options doesn't have to be static...

epenet · 2022-10-10T10:29:40Z

Point taken. I should have phrased it as static or usually unchanged after initialisation

frenck

LGTM

@MartinHjelmare Do you agree on the typing change that I suggested (and has been applied by @epenet)?

MartinHjelmare

Looks good!

epenet · 2022-10-10T12:17:43Z

I have added a breaking change section (for custom component developers) and a blog post PR.
Once this is merged I will check again all the HACS components.

frenck · 2022-10-10T12:19:58Z

Thanks, @epenet 👍

epenet requested a review from a team as a code owner September 21, 2022 11:00

homeassistant added the cla-signed label Sep 21, 2022

probot-home-assistant bot added code-quality integration: select small-pr PRs with less than 30 lines. labels Sep 21, 2022

epenet requested review from rytilahti, syssi, bieniu and frenck as code owners September 21, 2022 11:23

MartinHjelmare reviewed Sep 21, 2022

View reviewed changes

epenet commented Sep 21, 2022

View reviewed changes

MartinHjelmare approved these changes Sep 21, 2022

View reviewed changes

frenck reviewed Sep 21, 2022

View reviewed changes

epenet marked this pull request as draft September 21, 2022 13:04

epenet force-pushed the SelectEntityDescription branch from 14d6499 to 7fd9e05 Compare September 21, 2022 14:00

epenet marked this pull request as ready for review September 21, 2022 15:05

epenet mentioned this pull request Sep 22, 2022

Rename property in Plugwise EntityDescription #78935

Merged

23 tasks

frenck self-requested a review September 26, 2022 08:54

epenet force-pushed the SelectEntityDescription branch from 5c89ede to af3dfb1 Compare September 29, 2022 05:25

Add current_option to SelectEntityDescription

296ace2

Revert "Add current_option to SelectEntityDescription"

9f9a91e

This reverts commit 296ace2.

frenck requested a review from MartinHjelmare October 10, 2022 10:35

frenck approved these changes Oct 10, 2022

View reviewed changes

MartinHjelmare approved these changes Oct 10, 2022

View reviewed changes

epenet mentioned this pull request Oct 10, 2022

Add blog post for SelectEntityDescription home-assistant/developers.home-assistant#1493

Merged

5 tasks

epenet added the breaking-change label Oct 10, 2022

frenck merged commit ca9bfc8 into home-assistant:dev Oct 10, 2022

epenet deleted the SelectEntityDescription branch October 10, 2022 12:22

github-actions bot locked and limited conversation to collaborators Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add options to SelectEntityDescription #78882

Add options to SelectEntityDescription #78882

epenet commented Sep 21, 2022 •

edited

Loading

probot-home-assistant bot commented Sep 21, 2022

MartinHjelmare Sep 21, 2022

epenet Sep 21, 2022

epenet Sep 21, 2022

MartinHjelmare Sep 21, 2022

epenet Sep 21, 2022

MartinHjelmare left a comment

frenck Sep 21, 2022 •

edited

Loading

MartinHjelmare Sep 21, 2022

frenck Oct 10, 2022 •

edited

Loading

frenck commented Sep 21, 2022 •

edited

Loading

epenet commented Sep 21, 2022

MartinHjelmare commented Sep 21, 2022

frenck commented Sep 21, 2022

epenet commented Sep 21, 2022

bouwew commented Sep 22, 2022 •

edited

Loading

epenet commented Sep 22, 2022

epenet commented Sep 28, 2022

frenck commented Sep 28, 2022

epenet commented Sep 28, 2022

epenet commented Oct 10, 2022

frenck commented Oct 10, 2022

epenet commented Oct 10, 2022 •

edited

Loading

frenck commented Oct 10, 2022 •

edited

Loading

MartinHjelmare commented Oct 10, 2022

epenet commented Oct 10, 2022

frenck commented Oct 10, 2022

epenet commented Oct 10, 2022 •

edited

Loading

frenck left a comment

MartinHjelmare left a comment

epenet commented Oct 10, 2022

frenck commented Oct 10, 2022

	options: list[str] = UNDEFINED # type: ignore[assignment]
	options: list[str] \| None = None

Add options to SelectEntityDescription #78882

Add options to SelectEntityDescription #78882

Conversation

epenet commented Sep 21, 2022 • edited Loading

Breaking change

Proposed change

HACS:

Type of change

Additional information

Checklist

probot-home-assistant bot commented Sep 21, 2022

MartinHjelmare Sep 21, 2022

Choose a reason for hiding this comment

epenet Sep 21, 2022

Choose a reason for hiding this comment

epenet Sep 21, 2022

Choose a reason for hiding this comment

MartinHjelmare Sep 21, 2022

Choose a reason for hiding this comment

epenet Sep 21, 2022

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

frenck Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

MartinHjelmare Sep 21, 2022

Choose a reason for hiding this comment

frenck Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

frenck commented Sep 21, 2022 • edited Loading

epenet commented Sep 21, 2022

MartinHjelmare commented Sep 21, 2022

frenck commented Sep 21, 2022

epenet commented Sep 21, 2022

bouwew commented Sep 22, 2022 • edited Loading

epenet commented Sep 22, 2022

epenet commented Sep 28, 2022

frenck commented Sep 28, 2022

epenet commented Sep 28, 2022

epenet commented Oct 10, 2022

frenck commented Oct 10, 2022

epenet commented Oct 10, 2022 • edited Loading

frenck commented Oct 10, 2022 • edited Loading

MartinHjelmare commented Oct 10, 2022

epenet commented Oct 10, 2022

frenck commented Oct 10, 2022

epenet commented Oct 10, 2022 • edited Loading

frenck left a comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

epenet commented Oct 10, 2022

frenck commented Oct 10, 2022

epenet commented Sep 21, 2022 •

edited

Loading

frenck Sep 21, 2022 •

edited

Loading

frenck Oct 10, 2022 •

edited

Loading

frenck commented Sep 21, 2022 •

edited

Loading

bouwew commented Sep 22, 2022 •

edited

Loading

epenet commented Oct 10, 2022 •

edited

Loading

frenck commented Oct 10, 2022 •

edited

Loading

epenet commented Oct 10, 2022 •

edited

Loading