Allow area, device, and entity selectors to optionally support multiple selections like target selector by r-t-s · Pull Request #63138 · home-assistant/core

r-t-s · 2021-12-31T14:59:42Z

Proposed change

Changes to the Area, Device, and Entity selectors to allow them to select multiple entities.
The user interface is similar to the Area, Device and Entity selector component of the target selector.
Single or multi select is configured with the optional multiple option on the selector configuration.
The selectors can now potentially return a list of area, device, or entity IDs.

This is motivated by the need to have multiple entity selector so a blueprint can trigger on multiple entities.
But brings a consistent UI for the Area, Device, Entity, and Target selector.

The associated User Interface PR #11059

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: #20961

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/helpers/selector.py

balloob · 2022-02-21T22:44:25Z

homeassistant/helpers/selector.py

@@ -125,6 +127,8 @@ class DeviceSelector(Selector):
            vol.Optional("model"): str,
            # Device has to contain entities matching this selector
            vol.Optional("entity"): EntitySelector.CONFIG_SCHEMA,


This selector is changing. Please inline the old config schema.

CC @emontnemery

balloob · 2022-02-21T22:44:31Z

homeassistant/helpers/selector.py

            vol.Optional("entity"): EntitySelector.CONFIG_SCHEMA,
            vol.Optional("device"): DeviceSelector.CONFIG_SCHEMA,


This selector is changing. Please inline the old config schema.

balloob · 2022-02-21T22:44:57Z

homeassistant/helpers/selector.py

        {
            vol.Optional("entity"): EntitySelector.CONFIG_SCHEMA,
            vol.Optional("device"): DeviceSelector.CONFIG_SCHEMA,
+            # Allow multiple area selection


Please remove comments. It's clear what multiple means 😄

balloob · 2022-02-21T22:46:06Z

Please update the __call__ methods of the selectors to validate that it's a list of valid entity/device/area. Also add tests for this. Your current tests are incorrect.

…le selections like target selector

frenck · 2022-02-22T19:16:35Z

@r-t-s Please do not squash commits, it makes it really hard to follow changes during the review. Thanks! 👍

balloob · 2022-02-22T20:26:04Z

homeassistant/helpers/selector.py

-                )
-
-            return entity_id
+            result = cv.entity_ids_or_uuids(data)


When multiple is set to true, we should only validate it as multiple entities, and only validate as single if multiple is false.

I was keeping with the interface for entity_id in a trigger. It accepts a single or a list. Currently in the selector I only return a single entity if one is selected, and a list if enabled and more than 1 is selected. I believe the entity_id in the target selector behaves the same way.

That is not always the case, selectors can also be used in places that can't handle lists.

They selector config must specify multiple and the user must pick more than one to get a list. So we should be OK.
The question is should we always return a list (even an empty list) if multiple is specified.

Yes we should always return a list if multiple is set to true and a string if not.

However, your current code is not checking if multiple is set, but just validates first as a single, then as multiple. We should just check the value of multiple.

balloob · 2022-02-22T20:26:31Z

homeassistant/helpers/selector.py

+        result: str | list[str]
+
+        def validate(e_or_u: str) -> None:
+            if "domain" in self.config:


Use guard clauses to keep the code readable.

if 'domain' not in self.config: return

balloob · 2022-02-22T20:26:46Z

homeassistant/helpers/selector.py

+
+        def validate(e_or_u: str) -> None:
+            if "domain" in self.config:
+                if valid_entity_id(e_or_u):


Why would you not raise if it's not a valid entity ID ?

Because it could be a uuid the previous validation was a little obscure ... and I needed to test each result in the list.

balloob · 2022-03-02T16:58:14Z

tests/components/blueprint/test_importer.py

                "integration": "zha",
                "manufacturer": "IKEA of Sweden",
                "model": "TRADFRI remote control",
+                "multiple": False,


Let's not update all tests but also leave some where we don't specify multiple, to verify the default is False.

That's exactly what happens; "multiple" has False as default value so it's added to the validated blueprint input 👍
The changes just adjusts the test because the validated blueprint input has changed.

balloob · 2022-03-02T16:59:10Z

homeassistant/helpers/selector.py

+
+        def validate(e_or_u: str) -> str:
+            if not valid_entity_id(e_or_u):
+                return e_or_u


Should we still verify it looks like a uuid ?

Oh nm you do that before passing to validate.

balloob · 2022-03-02T17:01:12Z

homeassistant/helpers/selector.py

+            return e_or_u
+
+        if not self.config["multiple"]:
+            return validate(cv.entity_id_or_uuid(cv.string(data)))


Looks like you could move cv.entity_id_or_uuid(cv.string(data)) inside validate too ?

Is cv.string necessary on top of cv.entity_id_or_uuid ?

balloob · 2022-03-02T23:58:18Z

homeassistant/helpers/selector.py

+            return validate(data)
+        if not isinstance(data, list):
+            raise vol.Invalid("Value should be a list")
+        return [validate(item) for item in data]


If you do vol.Schema([validate]) it will give a nicer error message, something like "value at index X invalid: XX". Not sure if that really matters.

Yeah, that's a bit better indeed 👍

before:
voluptuous.error.MultipleInvalid: Entity light.def456 belongs to domain light, expected sensor for dictionary value @ data['selection']
after:
voluptuous.error.MultipleInvalid: Entity light.def456 belongs to domain light, expected sensor @ data['selection'][1]

emontnemery

Thanks @r-t-s, this is great 👍

r-t-s requested a review from a team as a code owner December 31, 2021 14:59

homeassistant added the cla-signed label Dec 31, 2021

probot-home-assistant bot added core new-feature small-pr PRs with less than 30 lines. labels Dec 31, 2021

r-t-s changed the title ~~Allow area, device, and entity selectors to optionally support multile selections like target selector~~ Allow area, device, and entity selectors to optionally support multiple selections like target selector Dec 31, 2021

frenck reviewed Jan 4, 2022

View reviewed changes

homeassistant/helpers/selector.py Outdated Show resolved Hide resolved

r-t-s force-pushed the entity_selector branch from 6f84fba to 6226ae4 Compare January 4, 2022 22:03

r-t-s force-pushed the entity_selector branch from 6226ae4 to e7551f2 Compare February 21, 2022 13:45

balloob reviewed Feb 21, 2022

View reviewed changes

Allow area, device, and entity selectors to optionally support multip…

9ef8b40

…le selections like target selector

r-t-s force-pushed the entity_selector branch from e7551f2 to 9ef8b40 Compare February 22, 2022 18:51

balloob reviewed Feb 22, 2022

View reviewed changes

emontnemery added 2 commits March 2, 2022 15:22

Update according to code review comments

d778382

Adjust tests

fc76109

balloob reviewed Mar 2, 2022

View reviewed changes

Update according to review comments

fda993b

balloob reviewed Mar 2, 2022

View reviewed changes

balloob approved these changes Mar 2, 2022

View reviewed changes

Tweak error message for multiple entities

aea850f

emontnemery approved these changes Mar 3, 2022

View reviewed changes

emontnemery merged commit 1558028 into home-assistant:dev Mar 3, 2022

github-actions bot locked and limited conversation to collaborators Mar 4, 2022

r-t-s deleted the entity_selector branch March 7, 2022 14:24

r-t-s restored the entity_selector branch March 14, 2022 12:38

r-t-s deleted the entity_selector branch September 3, 2022 15:31

		vol.Optional("entity"): EntitySelector.CONFIG_SCHEMA,
		vol.Optional("device"): DeviceSelector.CONFIG_SCHEMA,

Uh oh!

Conversation

r-t-s commented Dec 31, 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!

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!

balloob commented Feb 21, 2022

Uh oh!

frenck commented Feb 22, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emontnemery Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emontnemery left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

r-t-s commented Dec 31, 2021 •

edited

Loading

emontnemery Mar 2, 2022 •

edited

Loading

balloob Mar 2, 2022 •

edited

Loading