-
-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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 KIWI Door Locks #14485
Conversation
bugfixes improved attribute display flake8 more style adjustments
flake8
reordered imports and flake8 attempt to pelase a very picky linter also pleasing pylint now :)
Successor of #12304 |
def setup_platform(hass, config, add_devices, discovery_info=None): | ||
"""Set up the KIWI lock platform.""" | ||
from kiwiki import KiwiClient | ||
kiwi = KiwiClient(config.get(CONF_USERNAME), config.get(CONF_PASSWORD)) |
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.
No need for get, you already require those parameters in the schema
def __init__(self, kiwi_lock, client): | ||
"""Initialize the lock.""" | ||
self._sensor = kiwi_lock | ||
self._device_attrs = None |
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.
No need to set it to None if you assign it four lines down
@property | ||
def is_locked(self): | ||
"""Return true if lock is locked.""" | ||
return True |
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.
Can it never be unlocked?
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.
No, there's no way to detect the actual locked state. Therefore, the assumed state is locked (devices automatically lock after unlock within 5 seconds)
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.
You could use this approach: #14419 Just change the state for some seconds.
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.
Please implement the suggested approach. It changes the state of the lock for a short period of time and makes automations easier.
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.
Ok
ATTR_CAN_INVITE: kiwi_lock.get('can_invite')} | ||
|
||
self._device_attrs.update(address) | ||
self._device_attrs.update({ |
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.
Why do extract lat/lng and add it again separately? You could update the content of address and add it to the list by **address:
self._device_attrs {
ATTR_ID: self.lock_I'd,
**address
}
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.
ok this can be nested structure? cool - i'll change it 👍
def setup_platform(hass, config, add_devices, discovery_info=None): | ||
"""Set up the KIWI lock platform.""" | ||
from kiwiki import KiwiClient | ||
kiwi = KiwiClient(config[CONF_USERNAME], config[CONF_PASSWORD]) |
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 guess that there will be a traceback if the credentials are wrong. The setup should fails otherwise the users end up with a non-working platform.
c7h/kiwiki_client#1 should be addressed upstream and the requirement updated. |
"""Set up the KIWI lock platform.""" | ||
from kiwiki import KiwiClient | ||
kiwi = KiwiClient(config[CONF_USERNAME], config[CONF_PASSWORD]) | ||
add_devices([KiwiLock(lock, kiwi) for lock in kiwi.get_locks()], True) |
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.
A log entry that there are no locks found would help the users. Also, the setup can be terminated if there are no locks.
@property | ||
def is_locked(self): | ||
"""Return true if lock is locked.""" | ||
if self._state is not None: |
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.
Remove this check. It doesn't add anything.
kiwi = KiwiClient(config[CONF_USERNAME], config[CONF_PASSWORD]) | ||
except KiwiException as e: | ||
_LOGGER.error(e.msg) | ||
return False |
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.
Don't return False
. Just return
.
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.
That is horrible to read. I prefer return False
- there is no room for misinterpretation.
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.
Nothing is checking the return value.
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.
Check the platform example -
False
is returned here on login failed. It makes sense even if its not checked because it improves the readability. https://developers.home-assistant.io/docs/en/creating_platform_example_light.html
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.
That example needs updating.
It doesn't improve readability. It gives the wrong impression what happens when the function returns.
It also makes the return inconsistent.
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.
Ok, I created an Issue (home-assistant/developers.home-assistant#21) and will change it to return
if not available_locks: | ||
# No locks found; abort setup routine. | ||
_LOGGER.info("No KIWI locks found in your account.") | ||
return False |
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.
Same as above.
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.
Sorry I missed this one.
_LOGGER.error("failed to open door") | ||
else: | ||
self._state = STATE_UNLOCKED | ||
async_call_later(self.hass, UNLOCK_MAINTAIN_TIME, |
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 is a callback so have to be run from within the event loop. Use hass.add_job
to schedule async_call_later
on the event loop.
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.
doesn't async_call_later
already do that for you?
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.
async_call_later
schedules the passed in callback yes, but async_call_later
itself needs to be called from within the event loop. The unlock
method here will not be run within the event loop but in a worker thread. So to make sure that async_call_later
is called from the event loop, we need to use hass.add_job
.
self.hass.add_job(
async_call_later, self.hass, UNLOCK_MAINTAIN_TIME, self.clear_unlock_state)
from kiwiki import KiwiClient, KiwiException | ||
try: | ||
kiwi = KiwiClient(config[CONF_USERNAME], config[CONF_PASSWORD]) | ||
except KiwiException as e: |
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.
Linter is failing here. Please use eg exc
instead of e
.
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.
that's sad.... 😢
Description:
Added support for the KIWI Door lock platform (https://kiwi.ki).
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5381
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.