Add Nuki battery percentage sensor#84968
Conversation
|
Hey there @pschmitt, @pvizeli, @pree, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Co-authored-by: Robert Svensson <Kane610@users.noreply.github.com>
|
@Kane610 Thanks for the quick review and suggestions. |
|
Missing test or add to coverage |
|
There is already a PR waiting for review regarding the device registry (#79806) |
|
Not sure how to proceed. |
|
When the other PR is merged you can merge the changes into your PR or vice versa. Just wanted to mention this :) |
|
Hello The better update will be integration of the https:// and not only http:// |
Sadly, the Nuki bridge doesn't support TLS. |
bbr111
left a comment
bbr111
left a comment
There was a problem hiding this comment.
tested in my local installation.
works - battery sensor is present hand shows percentage
|
Please remove the device registry related changes. Each PR should only do one thing. |
MartinHjelmare
changed the title
|
@MartinHjelmare I removed the device registry changes. |
|
Please either add tests or exclude the new module from coverage calculation in |
|
Done! |
| @property | ||
| def name(self): | ||
| """Return the name of the lock.""" | ||
| return "Battery" |
There was a problem hiding this comment.
Please use the _attr_ attributes to set the constant properties. Eg instead of defining a name property we set the class attribute _attr_name.
There was a problem hiding this comment.
Thanks for your feedback. I adapted the code.
Additional question: can I do the same for the ones that return data from the pynuki package?
Can this:
@property
def extra_state_attributes(self):
"""Return the device specific state attributes."""
data = {
ATTR_NUKI_ID: self._nuki_device.nuki_id,
}
return dataBe turned into this?
_attr_extra_state_attributes = {
ATTR_NUKI_ID: self._nuki_device.nuki_id,
}(Sorry, I'm new to HA development and haven't much experience with Python)
There was a problem hiding this comment.
Yes, but then we need to add an __init__ method and set the attribute as an instance attribute on the entity as it's bad practice to set a mutable object as a class attribute.
Also, this will only work if the attribute won't need updating after the entity is created. If the attribute needs updating, we either need to do that in a state update callback or keep using a property that is read when the entity state is updated.
So further changes for this is optional.
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Looks good!
Please link a docs PR in the PR description. We need to add the sensor platform to the front matter of the docs page:
https://www.home-assistant.io/integrations/nuki/
|
Wow, didn't think of that! You guys are very thorough ;) |
|
Thanks! |
8 participants
Proposed change
Nuki's API now exposes the battery percentage. Integrate this with Home Assistant.
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: