Qwikswitch async & updates#12641
Conversation
|
Merging today should give me 2 weeks in dev to test it out... |
| _LOGGER.error("Configure Qwikswitch component failed") | ||
| logging.getLogger(__name__).error( | ||
| "Configure Qwikswitch Light component failed") | ||
| return False |
There was a problem hiding this comment.
Nothing is checking this return value, just return.
|
|
||
| add_devices(qwikswitch.QSUSB['light']) | ||
| add_devices(hass.data['qwikswitch']['light']) | ||
| return True |
| qsusb = QSUsb(url, _LOGGER, dimmer_adjust) | ||
| def callback_value_changed(qsdevices, key, new): \ | ||
| # pylint: disable=unused-argument | ||
| """Update entiry values based on device change.""" |
| def callback_value_changed(qsdevices, key, new): \ | ||
| # pylint: disable=unused-argument | ||
| """Update entiry values based on device change.""" | ||
| entity = hass.data[DOMAIN].get(key) |
There was a problem hiding this comment.
The component should not know about the entity instances. That's a concern of the platforms. You should probably refactor this.
There was a problem hiding this comment.
You can eg use our dispatch helper to signal the entities to update state.
| return self._value | ||
| return self._qsusb[self._id, 1] > 0 | ||
|
|
||
| def turn_on(self, **kwargs): |
There was a problem hiding this comment.
Why have both turn_on and async_turn_on?
| item[QSDATA][QS_NAME].lower().endswith(' switch')): | ||
| item[QSDATA][QS_NAME] = item[QSDATA][QS_NAME][:-7] # Remove switch | ||
| new_dev = QSSwitch(_id, qsusb) | ||
| hass.data[DOMAIN]['switch'].append(new_dev) |
There was a problem hiding this comment.
The entities should not be stored in hass.data. It's ok to store an index referencing the entities though. Eg a map between entity_id and some other id pattern.
| _LOGGER.error("Configure Qwikswitch component") | ||
| logging.getLogger(__name__).error( | ||
| "Configure Qwikswitch Switch component failed") | ||
| return False |
There was a problem hiding this comment.
Nothing is checking this return value.
|
|
||
| add_devices(qwikswitch.QSUSB['switch']) | ||
| add_devices(hass.data['qwikswitch']['switch']) | ||
| return True |
| from homeassistant.helpers.aiohttp_client import async_get_clientsession | ||
| from homeassistant.helpers.discovery import load_platform | ||
| from homeassistant.components.light import ( | ||
| ATTR_BRIGHTNESS, SUPPORT_BRIGHTNESS, Light) |
There was a problem hiding this comment.
It's a bad pattern to import the Light entity class into the component. The platform specific entity classes should stay in the platforms.
If all platforms have the same logic regarding entity instantiation and you want to be DRY, you can eg instead create a common function used in setup_platform that takes the platform entity class as an argument to instantiate the entities. This function can be defined here in the component and be imported into the platforms.
| """Add lights from the main Qwikswitch component.""" | ||
| if discovery_info is None: | ||
| _LOGGER.error("Configure Qwikswitch component failed") | ||
| logging.getLogger(__name__).error( |
There was a problem hiding this comment.
Do we really need to log this? It will only happen if a user by mistake keeps a platform section for qwikswitch in the config under the light component.
|
Thanks for the review @MartinHjelmare - will use for the next PR Merged since it was open for a month and I would like to do some other changes... See lots of the things you mention is really carried over from the first version (my first PR to the project 😄) Before discovery, hass.data, dispatchers, async etc,- things have come a long way! |
ghost
removed
3 participants
Description:
Added async to qwikswitch library & HA component, but should hav no impact on user settings.
Old library used 2 dedicated threads for long poll & worker.
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:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.