Add asyncio support for Ebox #14183

titilambert · 2018-04-30T02:40:03Z

Description:

Update to the latest pyebox version

Related issue (if applicable): fixes #14003

Fixes: titilambert/pyebox#6

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

OttoWinter · 2018-04-30T05:00:31Z

homeassistant/components/sensor/ebox.py

@@ -62,25 +62,23 @@
 })


-def setup_platform(hass, config, add_devices, discovery_info=None):
+@asyncio.coroutine
+def async_setup_platform(hass, config, async_add_devices, discovery_info=None):


Below, you use the new async/await syntax and here the @asyncio.coroutine decorator. Please convert this method to async def async_setup_platform and replace yield from with await in the new method.

OttoWinter · 2018-04-30T05:01:45Z

homeassistant/components/sensor/ebox.py


    name = config.get(CONF_NAME)

    sensors = []
    for variable in config[CONF_MONITORED_VARIABLES]:
+        _LOGGER.error("EBOX %s", variable)


This is not an error log statement.

OttoWinter · 2018-04-30T05:02:32Z

homeassistant/components/sensor/ebox.py

        sensors.append(EBoxSensor(ebox_data, variable, name))

-    add_devices(sensors, True)
+    async_add_devices(sensors, False)


Why did you replace True with False here. Do you not want the sensors to automatically fill their value on startup?

OttoWinter · 2018-04-30T05:03:46Z

homeassistant/components/sensor/ebox.py

        except PyEboxError as exp:
            _LOGGER.error("Error on receive last EBox data: %s", exp)
-            return
+            return False


Nothin is checking the return value. Please just write return (also below)

OttoWinter · 2018-04-30T05:12:09Z

homeassistant/components/sensor/ebox.py


-# pylint: disable=import-error
-REQUIREMENTS = []  # ['pyebox==0.1.0'] - disabled because it breaks pip10
+REQUIREMENTS = ['pyebox==1.1.2']


Looking at the source for pyebox, I think timeouts are not done correctly. You're passing a timeout parameter to the .get and .post requests but that is not the overall timeout for the request. See this page for an example of timeouts in aiohttp: https://aiohttp.readthedocs.io/en/stable/

houndci-bot · 2018-04-30T20:58:04Z

homeassistant/components/sensor/ebox.py

@@ -62,25 +62,21 @@
 })


-def setup_platform(hass, config, add_devices, discovery_info=None):
+async def async_setup_platform(hass, config, async_add_devices, discovery_info=None):


line too long (85 > 79 characters)

houndci-bot · 2018-04-30T20:58:04Z

homeassistant/components/sensor/ebox.py

@@ -6,10 +6,10 @@
 For more details about this platform, please refer to the documentation at
 https://home-assistant.io/components/sensor.ebox/
 """
+import asyncio


'asyncio' imported but unused

titilambert · 2018-04-30T20:58:54Z

@OttoWinter Thanks for your review ! I fixed everything included timeout in the pyebox library (new release 1.1.3)

balloob · 2018-05-01T01:35:02Z

homeassistant/components/sensor/ebox.py

@@ -32,7 +31,7 @@
 DEFAULT_NAME = 'EBox'

 REQUESTS_TIMEOUT = 15
-SCAN_INTERVAL = timedelta(minutes=5)
+MIN_TIME_BETWEEN_UPDATES = timedelta(minutes=15)


Why change this from SCAN_INTERVAL? With SCAN_INTERVAL, Home Assistant will make sure to not call update more than once every 15 minutes per entity.

Setting both constants and a throttle is good if you want to have a hard limit on the interval and set a default interval for updates. Users can override SCAN_INTERVAL but not the throttle.

@balloob that is a data object, so it need a throttle

balloob · 2018-05-01T01:35:29Z

homeassistant/components/sensor/ebox.py

-        _LOGGER.error("Failed login: %s", error)
-        return False
+    httpsession = hass.helpers.aiohttp_client.async_get_clientsession()
+    ebox_data = EBoxData(username, password, httpsession)


Are you no longer validating that the user/pass are correct?

I wanted to speed up the startup. If user/password are not correct, we will get an error in the logs at the next check and all sensors will be empty.
Also, Ebox website are often in maintenance, so if you restart your hass daemon when the website is unavailable, the component is disabled and you have to restart hass later.
Are you correct with this, or do you prefer to keep the user/password check ?
Thanks

Keep the user/pass check, raise PlatformNotReady if it's not ready yet.

MartinHjelmare · 2018-05-01T08:35:49Z

homeassistant/components/sensor/ebox.py

        self.data = self.client.get_data()
+        return


Not necessary return.

MartinHjelmare · 2018-05-01T08:39:21Z

homeassistant/components/sensor/ebox.py

@@ -32,7 +31,7 @@
 DEFAULT_NAME = 'EBox'

 REQUESTS_TIMEOUT = 15
-SCAN_INTERVAL = timedelta(minutes=5)
+MIN_TIME_BETWEEN_UPDATES = timedelta(minutes=15)


Setting both constants and a throttle is good if you want to have a hard limit on the interval and set a default interval for updates. Users can override SCAN_INTERVAL but not the throttle.

houndci-bot · 2018-05-04T15:16:56Z

homeassistant/components/sensor/ebox.py

        self.data = self.client.get_data()
+        Return True


SyntaxError: invalid syntax

titilambert · 2018-05-04T21:22:14Z

I fixed my email, so the cla-error label can be removed

* Fix Ebox sensor * Fix home-assistant#14183 comments * Update ebox.py * Update ebox.py * Continue fixing comments

titilambert requested review from cdce8p, Danielhiversen, kellerza and a team as code owners April 30, 2018 02:40

homeassistant added the cla-signed label Apr 30, 2018

titilambert force-pushed the ebox branch from 446c268 to a53244f Compare April 30, 2018 02:41

homeassistant added the cla-signed label Apr 30, 2018

titilambert force-pushed the ebox branch from a53244f to 50696bf Compare April 30, 2018 02:42

homeassistant added the cla-signed label Apr 30, 2018

Fix Ebox sensor

5e7b5eb

titilambert force-pushed the ebox branch from 50696bf to 5e7b5eb Compare April 30, 2018 02:43

homeassistant added the cla-signed label Apr 30, 2018

OttoWinter reviewed Apr 30, 2018

View reviewed changes

homeassistant added the cla-signed label Apr 30, 2018

houndci-bot reviewed Apr 30, 2018

View reviewed changes

titilambert force-pushed the ebox branch from a82a487 to 564d2b2 Compare April 30, 2018 20:59

homeassistant added the cla-signed label Apr 30, 2018

Fix home-assistant#14183 comments

0c712d0

titilambert force-pushed the ebox branch from 564d2b2 to 0c712d0 Compare April 30, 2018 21:00

homeassistant added the cla-signed label Apr 30, 2018

balloob reviewed May 1, 2018

View reviewed changes

MartinHjelmare reviewed May 1, 2018

View reviewed changes

cdce8p removed their request for review May 2, 2018 08:13

Update ebox.py

9038413

homeassistant added the cla-signed label May 4, 2018

houndci-bot reviewed May 4, 2018

View reviewed changes

homeassistant/components/sensor/ebox.py Outdated

self.data = self.client.get_data()

Return True

Copy link

houndci-bot May 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyntaxError: invalid syntax

Update ebox.py

c3ddde2

homeassistant added cla-signed cla-error labels May 4, 2018

titilambert force-pushed the ebox branch from 9217164 to 1f1aeb1 Compare May 4, 2018 21:19

homeassistant added the cla-signed label May 4, 2018

titilambert force-pushed the ebox branch from 1f1aeb1 to 55fad0c Compare May 4, 2018 21:24

homeassistant added the cla-signed label May 4, 2018

home-assistant deleted a comment from homeassistant May 5, 2018

MartinHjelmare added cla-recheck and removed cla-error labels May 5, 2018

homeassistant added cla-signed and removed cla-recheck labels May 5, 2018

fabaff changed the title ~~Ebox~~ Add support for Ebox May 10, 2018

titilambert force-pushed the ebox branch from 55fad0c to f843e8e Compare May 26, 2018 13:05

homeassistant added the cla-signed label May 26, 2018

titilambert changed the title ~~Add support for Ebox~~ Add asyncio support for Ebox May 26, 2018

Continue fixing comments

eba3511

titilambert force-pushed the ebox branch from f843e8e to eba3511 Compare May 26, 2018 13:12

homeassistant added the cla-signed label May 26, 2018

balloob approved these changes May 29, 2018

View reviewed changes

balloob merged commit 4105429 into home-assistant:dev May 29, 2018

balloob mentioned this pull request Jun 8, 2018

0.71.0 #14876

Merged

girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018

Add asyncio support for Ebox (home-assistant#14183)

444b48f

* Fix Ebox sensor * Fix home-assistant#14183 comments * Update ebox.py * Update ebox.py * Continue fixing comments

home-assistant locked and limited conversation to collaborators Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asyncio support for Ebox #14183

Add asyncio support for Ebox #14183

titilambert commented Apr 30, 2018 •

edited

OttoWinter Apr 30, 2018

OttoWinter Apr 30, 2018

OttoWinter Apr 30, 2018

OttoWinter Apr 30, 2018

OttoWinter Apr 30, 2018

houndci-bot Apr 30, 2018

houndci-bot Apr 30, 2018

titilambert commented Apr 30, 2018

balloob May 1, 2018

MartinHjelmare May 1, 2018

pvizeli May 4, 2018

balloob May 1, 2018

titilambert May 4, 2018

balloob May 5, 2018

titilambert May 26, 2018

MartinHjelmare May 1, 2018

MartinHjelmare May 1, 2018

houndci-bot May 4, 2018

titilambert commented May 4, 2018

Add asyncio support for Ebox #14183

Add asyncio support for Ebox #14183

Conversation

titilambert commented Apr 30, 2018 • edited

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titilambert commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titilambert commented May 4, 2018

titilambert commented Apr 30, 2018 •

edited