-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate Wyoming satellite to Assist satellite entity #128488
Migrate Wyoming satellite to Assist satellite entity #128488
Conversation
Hey there @balloob, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
||
# Use config entry id since only one satellite per entry is supported | ||
satellite_id = entry.entry_id | ||
device = dev_reg.async_get_or_create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we always create a device (with device_type="service" btw), and put all service entities linked to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(for a future PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Always" as in even if there isn't a satellite present (i.e., for a TTS service)?
@callback | ||
def async_get_configuration( | ||
self, | ||
) -> AssistSatelliteConfiguration: | ||
"""Get the current satellite configuration.""" | ||
raise NotImplementedError | ||
|
||
async def async_set_configuration( | ||
self, config: AssistSatelliteConfiguration | ||
) -> None: | ||
"""Set the current satellite configuration.""" | ||
raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong and an implementation failure on the assist satellite entity. It means now the methods can raise unexpectedly. We should have a default implementation for async_get_configuration
that returns a stub, and maybe a default set_configuration that raises NotImplementedError ? But even then, HA needs to know it cannot set a config, or it will show a UI for it maybe? Maybe the default config should include a "cannot set config" boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it should be changed, but not in this PR. It needs to be fixed in VoIP too.
Co-authored-by: Paulus Schoutsen <[email protected]>
Co-authored-by: Paulus Schoutsen <[email protected]>
Breaking change
Proposed change
Initial migration of Wyoming satellite to an Assist satellite entity. This does not add any new features yet, such as support for announcements.
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
.To help with the load of incoming pull requests: