Converted SABnzbd to a component #12915

jeradM · 2018-03-05T03:59:02Z

Description:

Converted SABnzbd sensor platform to a component with services that can be called to interact with the SABnzbd queue. This is a breaking change.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4836

Example entry for `configuration.yaml` (if applicable):

sabnzbd:
  api_key: YOUR_SABNZBD_API_KEY

Checklist:

The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. 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.

balloob · 2018-05-01T20:09:00Z

homeassistant/components/sensor/sabnzbd.py

-
-    @asyncio.coroutine
-    def async_update(self):
+    def update_state(self, args):


Mark this as @callback if it's an async method or call schedule_update_ha_state instead of the async version.

balloob · 2018-05-01T20:09:06Z

homeassistant/components/sensor/sabnzbd.py

-                self._state = self.sabnzbd_api.queue.get('noofslots_total')
-            else:
-                self._state = 'Unknown'
+        self._state = self._sabnzbd_api.get_queue_field(self._field_name)


Is this doing I/O ?

No I/O, just a dict lookup

Use None as default value to represent unknown state.

balloob · 2018-05-01T20:11:12Z

homeassistant/components/sabnzbd.py

+                                 schema=SPEED_LIMIT_SCHEMA)
+
+
+def request_configuration(hass, config, host):


Config entries is the new shizzle, configurator is old. You don't have to upgrade it in this PR, would be nice for a future PR ;)

Will do. When I started this config entries wasn't a thing yet, but I'll definitely convert this in the future.

balloob · 2018-05-01T20:11:43Z

homeassistant/components/sabnzbd.py

+            else:
+                await sab_api_data.async_set_queue_speed()
+
+    hass.services.async_register(DOMAIN, SERVICE_PAUSE, async_service_handler)


If you don't expect data, pass in vol.Schema({})

Will do. One question though just for my understanding: why does async_register() have a default value of schema=None? Would it make more sense for the default to be schema=vol.Schema({}) or to not have a default at all? Does schema=None have a special meaning?

balloob · 2018-05-01T20:12:51Z

homeassistant/components/sabnzbd.py

+    configurator = hass.components.configurator
+    # We got an error if this method is called while we are configuring
+    if host in _CONFIGURING:
+        configurator.notify_errors(_CONFIGURING[host],


You are inside an async context as you call this from async_setup_platform, this needs to be async_notify_errors

balloob · 2018-05-01T20:13:48Z

homeassistant/components/sabnzbd.py

+                                 schema=SPEED_LIMIT_SCHEMA)
+
+
+def request_configuration(hass, config, host):


If this is supposed to be called from an async context, prefix the name with async_

balloob · 2018-05-01T20:14:30Z

homeassistant/components/sabnzbd.py

+    uri_scheme = 'https' if use_ssl else 'http'
+    base_url = BASE_URL_FORMAT.format(uri_scheme, host, port)
+    if api_key is None:
+        conf = load_json(hass.config.path(CONFIG_FILE))


You cannot do I/O inside an async context 😮

Yield to the executor: conf = await hass.async_add_job(load_json, hass.config.path(CONFIG_FILE)

Same goes for writing.

balloob · 2018-05-01T20:15:41Z

homeassistant/components/sabnzbd.py

+                req_config = _CONFIGURING.pop(host)
+                configurator.async_request_done(req_config)
+
+            success()


You removed the call to hass.async_add_job from the previous code, and so you're doing I/O inside the event loop 😞

balloob

Quite some comments. Might want to brush up on asyncio

jeradM · 2018-05-02T03:12:24Z

These should (hopefully) all be fixed now.

I do think I may have screwed up your review a little bit by force pushing the updates. I added a commit a while back by fixing a merge conflict directly in the github ui and forgot I didn't have it locally. Then after rebasing upstream and adding commits I ended up with a bunch of conflicts so I just force pushed it. I didn't think it would be an issue since that one commit only touched discovery.py, but it looks like all your comments were still attached to that commit so I'm sorry if that makes it a PITA for you to review the most recent changes

balloob · 2018-05-02T17:20:54Z

homeassistant/components/sabnzbd.py

+        vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
+        vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
+        vol.Optional(CONF_SCAN_INTERVAL, default=DEFAULT_SCAN_INTERVAL):
+            cv.time_period_seconds,


Use cv.time_period

balloob · 2018-05-02T17:21:34Z

homeassistant/components/sabnzbd.py

+        vol.Optional(CONF_HOST, default=DEFAULT_HOST): cv.string,
+        vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
+        vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
+        vol.Optional(CONF_SCAN_INTERVAL, default=DEFAULT_SCAN_INTERVAL):


I don't see any reason to make this configurable. Let's pick a sane default and that's it.

@balloob do you think 30 seconds is a good default? I made it configurable because, for me, SABnzbd is idle probably 90% of the time and I would be happy only getting updates every 10 minutes or so. However, I can imagine some of the more "power users" would rather have more up-to-date download statistics for use in their automations to, for instance, determine when to reduce download speed or pause downloading altogether to prevent exceeding data caps. With the availability of gigabit+ residential connections, a 30 second update interval could mean downloading 3-4 GB between updates.

balloob · 2018-05-02T17:28:29Z

homeassistant/components/sabnzbd.py

+                conf[host] = {CONF_API_KEY: api_key}
+                save_json(hass.config.path(CONFIG_FILE), conf)
+                req_config = _CONFIGURING.pop(host)
+                configurator.async_request_done(req_config)


This method is synchronous, so you cannot call an async method.

MartinHjelmare · 2018-05-02T22:56:25Z

homeassistant/components/sensor/sabnzbd.py

        self._unit_of_measurement = SENSOR_TYPES[sensor_type][1]

+        async_dispatcher_connect(hass, SIGNAL_SABNZBD_UPDATED,


Move this to async_added_to_hass. Calling the state update before the entity has been added to home assistant will error.

MartinHjelmare · 2018-05-02T22:57:08Z

homeassistant/components/sensor/sabnzbd.py



 class SabnzbdSensor(Entity):
    """Representation of an SABnzbd sensor."""

-    def __init__(self, sensor_type, sabnzbd_api, client_name):
+    def __init__(self, hass, sensor_type, sabnzbd_api_data, client_name):


Don't pass in hass. It will be set on the entity when it has been added to home assistant.

jeradM · 2018-05-05T03:46:46Z

@balloob @MartinHjelmare I am ready to push changes, but my build is going to fail due to #14281. Should I push as-is so I don't screw up the review, or go ahead and rebase first?

MartinHjelmare · 2018-05-05T07:31:07Z

It's ok to rebase. Try to avoid squashing the commits though. Then review will not be affected.

jeradM · 2018-05-06T00:11:30Z

Thanks @MartinHjelmare. This should be cleaned up now. I think I finally have my head around the async stuff

MartinHjelmare · 2018-05-06T08:59:04Z

homeassistant/components/sabnzbd.py

+        conf = await hass.async_add_job(load_json,
+                                        hass.config.path(CONFIG_FILE))
+
+        if conf.get(base_url, {}).get(CONF_API_KEY):


api_key = conf.get(base_url, {}).get(CONF_API_KEY, '')

MartinHjelmare · 2018-05-06T09:08:33Z

homeassistant/components/sabnzbd.py

+        """Handle configuration changes."""
+        api_key = data.get(CONF_API_KEY)
+        sab_api = SabnzbdApi(host, api_key)
+        if await async_check_sabnzbd(sab_api):


Invert this check and return if it's true, to make it a guard clause. Then you can outdent the following code.

MartinHjelmare · 2018-05-06T09:15:43Z

homeassistant/components/sabnzbd.py

+        elif service.service == SERVICE_RESUME:
+            await sab_api_data.async_resume_queue()
+        elif service.service == SERVICE_SET_SPEED:
+            speed = service.data.get(ATTR_SPEED, None)


None is the default value returned if the key is missing in the dict.

MartinHjelmare · 2018-05-06T09:16:46Z

homeassistant/components/sabnzbd.py

+        elif service.service == SERVICE_SET_SPEED:
+            speed = service.data.get(ATTR_SPEED, None)
+            if speed is None:
+                speed = '100'


Move this default value to the service schema.

MartinHjelmare · 2018-05-06T09:22:50Z

homeassistant/components/sensor/sabnzbd.py

-                self._state = self.sabnzbd_api.queue.get('noofslots_total')
-            else:
-                self._state = 'Unknown'
+        self._state = self._sabnzbd_api.get_queue_field(self._field_name)


Use None as default value to represent unknown state.

MartinHjelmare · 2018-05-06T09:24:19Z

homeassistant/components/sensor/sabnzbd.py

+        self._state = self._sabnzbd_api.get_queue_field(self._field_name)
+
+        if self._type == 'speed':
+            self._state = round(float(self._state) / 1024, 1)


This will error if state is not a number.

@MartinHjelmare I can add error handling here to be safe, but this should always be a number. This method will only get called by the dispatcher signal after a successful update and the library will always populate these numeric fields with a numeric value. They will always either be a number from the SABnzbd API or get converted to a numeric value by the pysabnzbd library.

Ok, fine to keep it as is.

Ok cool thanks

MartinHjelmare

Final comment. Otherwise looks good!

MartinHjelmare · 2018-05-06T20:10:22Z

homeassistant/components/sensor/sabnzbd.py

+async def async_setup_platform(hass, config, async_add_devices,
+                               discovery_info=None):
+    """Set up the SABnzbd sensors."""
+    sab_api_data = hass.data[DATA_SABNZBD]


We only set up the sensor platform from the component now, correct? So otherwise we don't want to do anything here. Add a guard clause that checks if discovery_info is None and returns if so.

if discovery_info is None: return ...

Yep, that's correct. Fixed

MartinHjelmare

Great!

jeradM requested a review from andrey-git as a code owner March 5, 2018 03:59

homeassistant added integration: discovery new-platform platform: sensor.sabnzbd integration: sabnzbd cla-signed labels Mar 5, 2018

jeradM mentioned this pull request Mar 5, 2018

Updated documentation for SABnzbd. home-assistant/home-assistant.io#4836

Merged

2 tasks

jeradM force-pushed the sabnzbd branch from 9b18991 to f3e31b4 Compare March 10, 2018 21:47

homeassistant added the cla-signed label Mar 10, 2018

balloob added the breaking-change label May 1, 2018

balloob reviewed May 1, 2018

View reviewed changes

balloob requested changes May 1, 2018

View reviewed changes

jeradM force-pushed the sabnzbd branch from 905a133 to e0ec18f Compare May 2, 2018 02:39

homeassistant added the cla-signed label May 2, 2018

balloob reviewed May 2, 2018

View reviewed changes

MartinHjelmare requested changes May 2, 2018

View reviewed changes

jeradM added 2 commits May 5, 2018 07:21

Converted SABnzbd to a component

916b7f7

fixed async issues

3e689ff

jeradM force-pushed the sabnzbd branch from e0ec18f to a258050 Compare May 5, 2018 14:24

homeassistant added the cla-signed label May 5, 2018

Made sabnzbd scan interval static. More async fixes.

d49dace

jeradM force-pushed the sabnzbd branch from a258050 to d49dace Compare May 6, 2018 06:45

homeassistant added the cla-signed label May 6, 2018

MartinHjelmare requested changes May 6, 2018

View reviewed changes

Sabnzbd component code cleanup

ef00831

homeassistant added the cla-signed label May 6, 2018

MartinHjelmare reviewed May 6, 2018

View reviewed changes

Skip sensor platform setup if discovery_info is None

ee75439

MartinHjelmare approved these changes May 7, 2018

View reviewed changes

MartinHjelmare merged commit e60d066 into home-assistant:dev May 7, 2018

jeradM deleted the sabnzbd branch May 11, 2018 19:47

balloob mentioned this pull request May 28, 2018

0.70.0 #14657

Merged

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

ghost removed the platform: sensor.sabnzbd label Mar 21, 2019

		schema=SPEED_LIMIT_SCHEMA)


		def request_configuration(hass, config, host):

		self._unit_of_measurement = SENSOR_TYPES[sensor_type][1]

		async_dispatcher_connect(hass, SIGNAL_SABNZBD_UPDATED,

Converted SABnzbd to a component #12915

Converted SABnzbd to a component #12915

Conversation

jeradM commented Mar 5, 2018 • edited

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

balloob May 1, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment • edited by fabaff

Choose a reason for hiding this comment

jeradM commented May 2, 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

MartinHjelmare May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeradM commented May 5, 2018

MartinHjelmare commented May 5, 2018

jeradM commented May 6, 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

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

jeradM commented Mar 5, 2018 •

edited

Example entry for `configuration.yaml` (if applicable):

balloob May 1, 2018 •

edited

balloob left a comment •

edited by fabaff

MartinHjelmare May 2, 2018 •

edited