Refactor Ring data handling#30777
Conversation
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
It's still not clear to me why it's important to allow platforms to decide if we should fetch data for the devices. I don't see where the number of requests could be dynamic based on a decision in a platform.
| self._unsub_interval = None | ||
|
|
||
| def refresh_all(self, _): | ||
| def refresh_all(self, _=None): |
There was a problem hiding this comment.
Why is this not an async interface? We seem to be calling this from async context and two out of three calls inside are async.
There was a problem hiding this comment.
Jumping into the executor 10 times inside a loop is a lot of overhead of jumping around between async and sync. It's in that case better to run in sync and just schedule the callbacks to be called.
| self._active_alert = next( | ||
| ( | ||
| alert | ||
| for alert in self._ring.active_alerts() |
There was a problem hiding this comment.
Is this just accessing a local data cache?
| self.devices[device.device_id]["task"] = asyncio.current_task() | ||
| self.devices[device.device_id][ | ||
| "data" | ||
| ] = await self.hass.async_add_executor_job(self.update_method, device) |
There was a problem hiding this comment.
Does tracking a device also need to update the data? Couldn't we just let the consumer get the data from the cache?
Why do we need to let the platforms track devices? We already know what devices there are. We could let the component control the device interface completely and just have platforms get data as needed from the cache.
There was a problem hiding this comment.
If an entity is disabled, we wouldn't want to fetch that data.
* upstream/dev: (82 commits) Add support for vacuums to Alexa. (home-assistant#30764) Refactor Ring data handling (home-assistant#30777) Restore unit_of_measurement from entity registry (home-assistant#30780) Update pyubee to 0.8 (home-assistant#30785) Update emulated_roku to 0.1.9 (home-assistant#30791) Add Config Flow support, Device Registry support, available property to vizio component (home-assistant#30653) Allow input_* and timer component setup without config (home-assistant#30772) Search: Add search to default config and don't resolve area (home-assistant#30762) [ci skip] Translation update Use storage based collections for Timer platform (home-assistant#30765) Upgrade youtube_dl to version 2020.01.15 (home-assistant#30767) Whitelist Frenck for release Hass.io allow to reset password with CLI (home-assistant#30755) Revert home-assistant#29701 (home-assistant#30766) Add Safe Mode (home-assistant#30723) Update Ring to 0.6.0 (home-assistant#30748) Add support for the voltage sensor on the greeneye GEM (home-assistant#30484) Fix supported_features in MQTT fan (home-assistant#28680) Fix small typo in alarmdotcom component (home-assistant#30758) bump aiokef to 0.2.5 which uses locks (home-assistant#30753) ...
* Refactor Ring data handling * Add async_ to methods
4 participants
Description:
One final PR for Ring to clean up the data handling, inspired by comments from @MartinHjelmare.
Moves all data handling to global objects. They are either global endpoint based or per-device based.
Extract a generic RingEntityMixin to keep implementations simple.
Thanks to @dshokouhi for being able to test with his Ring account.
Checklist:
tox. Your PR cannot be merged unless tests passIf the code does not interact with devices: