-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
Add support for dormakaba dKey locks #87501
Conversation
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
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 don't know much about the bluetooth layers but the general implementation looks good!
async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: | ||
"""Unload a config entry.""" | ||
if unload_ok := await hass.config_entries.async_unload_platforms(entry, PLATFORMS): | ||
data: DormakabaDkeyData = hass.data[DOMAIN].pop(entry.entry_id) |
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.
Observation: The naming throws me off here, data I would expect be some data description, not a class with connection properties.
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.
The class is a container of data which the integration stores in hass.data[], what would be a better name?
Maybe DkeyConfigEntryData
?
bf7337b
to
65781a4
Compare
""" | ||
if self._async_in_progress(include_uninitialized=True): | ||
return self.async_abort(reason="no_additional_devices_found") | ||
return self.async_abort(reason="no_devices_found") |
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.
What if the discovery hasn't run yet? Then we should be able to force a discovery here, I think.
I don't think it's helpful for the user to just abort their effort to add the integration.
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.
There's no way to force a Bluetooth discovery, it runs continuously in the background.
I do agree the UX is not great, and I think that should be handled by improving the "add integration" dialog for bluetooth integrations which rely on Home Assistant's bluetooth discovery filter.
It could for example be updated so it shows a spinner + a text saying "Looking for compatible bluetooth devices", with the list of matching discovery flows updating while the dialog is open.
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 re-added the user flow for now so this integration is similar to other bluetooth integrations.
This reverts commit 0ef9d1c.
Proposed change
Add support for dormakaba dKey locks
Type of change
Additional information
Checklist
black --fast 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: