-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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 UpCloud platform #12011
Add UpCloud platform #12011
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.
Great work! 👍
Not an official reviewer, but leave you some food for thought
homeassistant/components/upcloud.py
Outdated
return False | ||
|
||
hass.data[DATA_UPCLOUD] = UpCloud(manager) | ||
hass.data[DATA_UPCLOUD].update() |
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.
Calling update() here will delay HA launch
Can you find away around it ?
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.
Not sure. In cases where there are only switches or binary sensors configured, this update() could just be removed, the component will call it whenever it needs to refresh. But when there are both switches and binary sensors configured, one of them will end up being configured without the data (because of the throttling) on start and thus shows incorrect/missing data.
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.
Looking closer, the UpCloud object is shared by both switches and binary sensors, so the servers list (which is the only thing in update()) will be shared.
But what about using homeassistant.helpers.dispatcher ?
homeassistant/components/upcloud.py
Outdated
return False | ||
|
||
hass.data[DATA_UPCLOUD] = UpCloud(manager) | ||
hass.data[DATA_UPCLOUD].update() |
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.
Looking closer, the UpCloud object is shared by both switches and binary sensors, so the servers list (which is the only thing in update()) will be shared.
But what about using homeassistant.helpers.dispatcher ?
Converted to Entity, use helpers.dispatcher. Maybe ready for inclusion now? |
Awesome 🎉 |
try: | ||
return STATE_MAP.get(self.data.state, STATE_UNKNOWN) | ||
except AttributeError: | ||
return STATE_UNKNOWN |
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.
Don't use STATE_UNKNOWN
. Return None
if state is not known.
|
||
def _update_callback(self): | ||
"""Call update method.""" | ||
self.schedule_update_ha_state(True) |
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 could be made async by using async_schedule_update_ha_state
.
async_dispatcher_connect( | ||
self.hass, SIGNAL_UPDATE_UPCLOUD, self._update_callback) | ||
|
||
def _update_callback(self): |
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 could be made async by annotating the method with @callback
from core.py.
|
||
def __init__(self, upcloud, uuid): | ||
"""Initialize a new UpCloud server switch.""" | ||
UpCloudServerEntity.__init__(self, upcloud, uuid) |
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.
super().__init__(upcloud, uuid)
|
||
def __init__(self, upcloud, uuid): | ||
"""Initialize a new UpCloud sensor.""" | ||
UpCloudServerEntity.__init__(self, upcloud, uuid) |
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.
super().__init__(upcloud, uuid)
Review comments addressed in #12835. Making the suggested constructor changes would have provoked pylint warnings, so I removed them altogether. |
Description:
Add UpCloud platform.
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4543
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:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass