Add dreo integration fan #117789
Add dreo integration fan #117789
Conversation
There was a problem hiding this comment.
The repository seems to be empty:
https://github.com/dreo-team/hscloud
Could you take a look?
../Frenck
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Modify PR document link |
|
repository: https://github.com/dreo-team/hscloud, has been pushed |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Hey home-assistant! |
For more information about our Pull Request process, please see: https://developers.home-assistant.io/docs/review-process Thanks ../Frenck |
This comment has been minimized.
This comment has been minimized.
|
@dreo-team please dit not ask for review, for more information about the review process check the page I linked above. Thanks! |
There was a problem hiding this comment.
Some initial comment to get the review processes going. I've left some quick picks and some larger comments.
Additionally:
- The translation string (
strings.json) is missing. - We require integrations to provide tests for config flows, with a 100% test coverage. Please add tests.
| @@ -1733,6 +1733,7 @@ omit = | |||
| homeassistant/components/zwave_me/sensor.py | |||
| homeassistant/components/zwave_me/siren.py | |||
| homeassistant/components/zwave_me/switch.py | |||
| homeassistant/components/dreo/fan.py | |||
There was a problem hiding this comment.
Please keep this file alphabetically sorted
| from hscloud.hscloud import HsCloud | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
| _LOGGER.setLevel(logging.INFO) |
There was a problem hiding this comment.
This shouldn't be controlled by the integration.
| _LOGGER.setLevel(logging.INFO) |
| login = await hass.async_add_executor_job(manager.login) | ||
| if not login: |
There was a problem hiding this comment.
login seems to be unused otherwise.
| login = await hass.async_add_executor_job(manager.login) | |
| if not login: | |
| if not await hass.async_add_executor_job(manager.login): |
| manager = HsCloud(username, password) | ||
| login = await hass.async_add_executor_job(manager.login) | ||
| if not login: | ||
| _LOGGER.error("Unable to login to the Dreo server") |
There was a problem hiding this comment.
- Should this raise an exception instead?
- This should ideally differentiate between an network/connection error and an authentication error (username/password doesn't match). Different exceptions can be raised for those cases.
| platforms = [] | ||
| fan_devices = [] | ||
| devices = await hass.async_add_executor_job(manager.get_devices) | ||
| for device in devices: | ||
| _platforms = PLATFORMS_CONFIG.get(device.get("model")) | ||
| for platform in _platforms: | ||
| if platform == FAN: | ||
| fan_devices.append(device) | ||
|
|
||
| platforms.extend(_platforms) | ||
|
|
||
| platforms = list(set(platforms)) |
There was a problem hiding this comment.
Shouldn't we just set up all platforms instead? And let the platform decide if it should add entities for a given device?
| _LOGGER.error(mask_error) | ||
| raise ValueError( | ||
| f"{exc}" | ||
| ) |
There was a problem hiding this comment.
These should raise an HomeAssistantError instead.
| from typing import Any | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
| _LOGGER.setLevel(logging.INFO) |
There was a problem hiding this comment.
| _LOGGER.setLevel(logging.INFO) |
| "ssdp": [], | ||
| "zeroconf": [], | ||
| "homekit": {}, | ||
| "dependencies": [], |
There was a problem hiding this comment.
| "ssdp": [], | |
| "zeroconf": [], | |
| "homekit": {}, | |
| "dependencies": [], |
| { | ||
| "domain": "dreo", | ||
| "name": "Dreo", | ||
| "version": "1.0.0", |
There was a problem hiding this comment.
Versions are not used by built-in integrations.
| "version": "1.0.0", |
| "homekit": {}, | ||
| "dependencies": [], | ||
| "codeowners": [ | ||
| "@kane" |
Breaking change
Added dreo integration fan
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
Link to documentation PR: home-assistant/home-assistant.io#32869
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: