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 Leviton Decora Smart WiFi Device Platform #8529
Conversation
@tlyakhov, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @michaelarnauts to be potential reviewers. |
|
||
This is the only method that should fetch new data for Home Assistant. | ||
""" | ||
self._switch = self._session.iot_switch_data(self._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.
no newline at end of file
|
||
return self.call_api("/IotSwitches/%s" % switch_id, payload, "put") | ||
|
||
class DecoraWifiLight(Light): |
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.
expected 2 blank lines, found 1
if brightness is not None: | ||
payload["brightness"] = brightness | ||
|
||
return self.call_api("/IotSwitches/%s" % switch_id, payload, "put") |
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.
trailing whitespace
def iot_switch_data(self, switch_id): | ||
return self.call_api("/IotSwitches/%s" % switch_id, None, "get") | ||
|
||
def iot_switch_update(self, switch_id, power = None, brightness = 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.
unexpected spaces around keyword / parameter equals
|
||
def iot_switch_data(self, switch_id): | ||
return self.call_api("/IotSwitches/%s" % switch_id, None, "get") | ||
|
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.
blank line contains whitespace
"email": email, | ||
"password": password, | ||
"clientId": "levdb-echo-proto", # from myLeviton App | ||
"registeredVia": "myLeviton" # from myLeviton App |
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.
at least two spaces before inline comment
payload = { | ||
"email": email, | ||
"password": password, | ||
"clientId": "levdb-echo-proto", # from myLeviton App |
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.
at least two spaces before inline comment
|
||
return json.loads(response.text) | ||
|
||
def login(self, email, 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.
trailing whitespace
_LOGGER.error("myLeviton API call (%s) failed: %s, %s", api, response.status_code, response.body) | ||
return None | ||
|
||
return json.loads(response.text) |
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.
trailing whitespace
response = getattr(self._session, method)(uri, data=payload_json) | ||
|
||
if response.status_code != 200 and response.status_code != 204: | ||
_LOGGER.error("myLeviton API call (%s) failed: %s, %s", api, response.status_code, response.body) |
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.
line too long (109 > 79 characters)
|
||
if response.status_code != 200 and response.status_code != 204: | ||
_LOGGER.error("myLeviton API call (%s) failed: %s, %s", | ||
api, response.status_code, response.body) |
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.
continuation line under-indented for visual indent
else: | ||
payload_json = '' | ||
|
||
response = getattr(self._session, method)(uri, data=payload_json) |
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.
trailing whitespace
uri = LEVITON_ROOT + api | ||
|
||
if payload is not None: | ||
payload_json = json.dumps(payload) |
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.
trailing whitespace
|
||
def call_api(self, api, payload = None, method = "get"): | ||
if method != "get" and method != "post" and method != "put": | ||
raise ValueError("Tried DecoraWifiSession.call_api with bad method: %s" % method) |
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.
line too long (93 > 79 characters)
self._email = None | ||
self._password = None | ||
|
||
def call_api(self, api, payload = None, method = "get"): |
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.
unexpected spaces around keyword / parameter equals
password = config.get(CONF_PASSWORD) | ||
|
||
|
||
persistent_notification = loader.get_component('persistent_notification') |
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.
too many blank lines (2)
NOTIFICATION_ID = "leviton_notification" | ||
NOTIFICATION_TITLE = 'myLeviton Decora Setup' | ||
|
||
def setup_platform(hass, config, add_devices, discovery_info=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.
expected 2 blank lines, found 1
}) | ||
|
||
""" The myLeviton API also supports some sort of socket communication... | ||
Probably better for the polling/update comms, but I haven't really looked at 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.
line too long (84 > 79 characters)
import voluptuous as vol | ||
|
||
from homeassistant.components.light import ATTR_BRIGHTNESS, Light, PLATFORM_SCHEMA, SUPPORT_BRIGHTNESS | ||
from homeassistant.const import CONF_USERNAME, CONF_PASSWORD, EVENT_HOMEASSISTANT_STOP |
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.
line too long (86 > 79 characters)
|
||
import voluptuous as vol | ||
|
||
from homeassistant.components.light import ATTR_BRIGHTNESS, Light, PLATFORM_SCHEMA, SUPPORT_BRIGHTNESS |
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.
line too long (102 > 79 characters)
else: | ||
payload_json = '' | ||
|
||
response = getattr(self._session, method)(uri, data=payload_json) |
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.
trailing whitespace
}) | ||
|
||
""" The myLeviton API also supports some sort of socket communication... | ||
Probably better for the polling/update comms, but I haven't really looked at 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.
line too long (80 > 79 characters)
vol.Required(CONF_PASSWORD): cv.string, | ||
}) | ||
|
||
""" |
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.
trailing whitespace
|
||
add_devices(DecoraWifiLight(session, switch) for switch in all_switches) | ||
|
||
"""Listen for the stop event and log out.""" |
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.
IndentationError: unindent does not match any outer indentation level
@@ -0,0 +1,237 @@ | |||
""" |
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.
IndentationError: unindent does not match any outer indentation level
|
||
def update(self): | ||
"""Fetch new state data for this switch.""" | ||
self._switch = self._session.iot_switch_data(self._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.
no newline at end of file
email = config.get(CONF_USERNAME) | ||
password = config.get(CONF_PASSWORD) | ||
|
||
persistent_notification = loader.get_component('persistent_notification') |
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
if success is None: | ||
message = 'Failed to log into myLeviton Services. Check credentials.' | ||
_LOGGER.error(message) | ||
persistent_notification.create(hass, |
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.
use hass.persistent_notification.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.
other places in the code use hass.components.persistent_notification.create(...)
, is that what you mean?
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 are right. That is a very new feature :)
at it. | ||
""" | ||
LEVITON_ROOT = 'https://my.leviton.com/api' | ||
DOMAIN = 'myLeviton' |
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.
domain need to be: decora_wifi
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.
Or you can remove it, you need no DOMAIN on platform.
The myLeviton API also supports some sort of socket communication... | ||
Probably better for the polling/update comms, but I haven't really looked | ||
at 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.
Remove that comment
import homeassistant.helpers.config_validation as cv | ||
import homeassistant.loader as loader | ||
|
||
REQUIREMENTS = [] |
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
|
||
import voluptuous as vol | ||
|
||
from homeassistant.components.light import (ATTR_BRIGHTNESS, |
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.
Shoul look like:
bla bla (
fdsjl, sdfjlsd, fsjdlf)
|
||
import json | ||
import logging | ||
import requests |
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.
move that above to voluptuous
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, don't understand this comment - it's already above voluptuous. Can you clarify please?
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.
BLOCK CORE IMPORT
BLOCK 3-PARTY IMPORT
BLOCK HASS IMPORT
return False | ||
|
||
# Save the session for logging out when HA is stopped. | ||
hass.data[DOMAIN] = session |
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 need not save that. use session
inside stop function, that will work.
@pvizeli fixed. Please review. |
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 remove all old style string format % from code
login_json = self.call_api('/Person/login', payload, 'post') | ||
|
||
if login_json is None: | ||
return 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.
only 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.
Seems like bad practice. See accepted answer here: https://stackoverflow.com/questions/15300550/python-return-return-none-and-no-return-at-all
"""Logout of LCS.""" | ||
if self._user_id is None: | ||
_LOGGER.info("Tried to log out, wasn't logged in.") | ||
return 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.
only return
raise ValueError(msg % method) | ||
|
||
if self._user_id is None and api != '/Person/login': | ||
raise ValueError('Tried an API call without a login.') |
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 raise a exception that do you no catch on a other place.
# Sanity check parameters first... | ||
if method != 'get' and method != 'post' and method != 'put': | ||
msg = "Tried DecoraWifiSession.call_api with bad method: %s" | ||
raise ValueError(msg % method) |
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 raise a exception that do you no catch on a other place.
@balloob please confirm for internal API code |
|
||
def iot_switches(self, residence_id): | ||
"""Get Leviton switch objects.""" | ||
api = "/Residences/{0}/iotSwitches".format(residence_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.
This is too much API specific information. please extract it into a third party lib.
Separated non-HASS code out into library. Apologies for the delay, I'm a Python n00b :-p. Please review again. |
Awesome, great work ! 🐬 🎉 🌮 |
Congratulations on landing your first PR! 🍻 |
* Add Leviton Decora Smart WiFi Device Platform * Decora WiFi Code Review Fixes
Description:
Adds support for Leviton Decora Smart WiFi Devices (specifically DW6HD-1BZ) (see http://www.leviton.com/en/products/lighting-controls/decora-smart-with-wifi)
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3021
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
.