-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dreo integration fan #117789
base: dev
Are you sure you want to change the base?
Add dreo integration fan #117789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_LOGGER.setLevel(logging.INFO) |
"ssdp": [], | ||
"zeroconf": [], | ||
"homekit": {}, | ||
"dependencies": [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ssdp": [], | |
"zeroconf": [], | |
"homekit": {}, | |
"dependencies": [], |
{ | ||
"domain": "dreo", | ||
"name": "Dreo", | ||
"version": "1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Versions are not used by built-in integrations.
"version": "1.0.0", |
"homekit": {}, | ||
"dependencies": [], | ||
"codeowners": [ | ||
"@kane" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is 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: