Improve intent slot schema handling #116789
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, @synesthesiam, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
@synesthesiam Appreciate if you could check this PR. It is not big, but I see it as a prerequisite for #115464 |
Also accept dict.
| return vol.Schema({}) | ||
| if isinstance(self.slot_schema, vol.Schema): | ||
| return self.slot_schema | ||
| return vol.Schema(self.slot_schema) |
There was a problem hiding this comment.
return vol.Schema(self.slot_schema)
Given that the type of slot_schema is vol.Schema | None, should it just be return self.slot_schema?
There was a problem hiding this comment.
Although it is indeed annotated as vol.Schema | None, many of the integrations actually use a dict (see DynamicServiceIntentHandler as an example). Unless we want to require intents to switch to vol.Schema in this PR, I chose to explicitly cast it to vol.Schema here.
| }, | ||
| extra=vol.ALLOW_EXTRA, |
There was a problem hiding this comment.
It's critical that ALLOW_EXTRA is injected into the overall slot schema. There are slots that get automatically set based on context, such as the device's area. Not all intents support this, so they should ignore those slots instead of throwing validation errors.
There was a problem hiding this comment.
It is still happening in _slot_schema. Sorry, I didn't describe it properly.
Previously _slot_schema method did two things:
- Add extra slots (i.e. for
DynamicServiceHandler) - Replace each validator with
{"value": validator}, addingALLOW_EXTRA
This PR breaks it down into two separate methods, allowing us to override the first part only. It would give us the universal way to check which slots the intent expects, and avoid code duplication.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
A little refactoring of
IntentHandlerclass. Instead of overriding of_slot_schemainDynamicServiceIntentHandler, introduce a neweffective_slot_schemathat is more natural to override.Currently
_slot_schemamethod does two things:{"value": validator}, addingvol.ALLOW_EXTRAThis PR breaks it down into two separate methods, allowing us to override the first part only. It would give us the universal way to check which slots the intent expects, and avoid code duplication.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: