-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
Introduced Ring binary sensors and refactored Ring component #6520
Conversation
- Added unittest for Ring binary_sensor. - Bumped ring_doorbell 3rd party module.
} | ||
|
||
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({ | ||
vol.Required(CONF_USERNAME): cv.string, |
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.
Instead of having each platform require credentials, please add a component to manage the credentials and share the Ring
instance by adding it to hass.data
(a dictionary intended for sharing exactly such objects)
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.
That is a good idea @balloob. I will work on that.
- Added unittest for Ring binary_sensor. - Bumped ring_doorbell 3rd party module.
from unittest import mock | ||
|
||
import homeassistant.components.binary_sensor as sensor | ||
from homeassistant.components.binary_sensor import ring |
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.
'homeassistant.components.binary_sensor.ring' imported but unused
homeassistant/components/ring.py
Outdated
|
||
ring = Ring(username, password) | ||
if ring.is_connected: | ||
RING = RingData(ring) |
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.
Use hass.data
to store RING
. Do not use globals.
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.
I see. Thanks for the heads.
|
||
def update(self): | ||
"""Get the latest data and updates the state.""" | ||
self._data.check_alerts(cache=self._cache) |
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.
Why do you need to pass in the cache? That is something that should be handled by the shared data class.
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 cache file is required for multiple sensors. It basically will save a pickle data file with the notification state shared by different sensors. The cache if required because it is shared on the object level and not on the account level which is shared by hass.data.
@@ -90,14 +65,15 @@ def setup_platform(hass, config, add_devices, discovery_info=None): | |||
|
|||
class RingSensor(Entity): | |||
"""A sensor implementation for Ring device.""" | |||
|
|||
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.
blank line contains whitespace
0659774
to
e61015a
Compare
@balloob code update. Please let know what you think now. I'm working to update the documentation to support the new sensors and how to configure it using the hub concept. Thank you! |
self._data.check_alerts(cache=self._cache) | ||
|
||
if self._data.alert: | ||
self._state = bool(self._sensor_type == |
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 ==
will always return a boolean, no need to make it one.
homeassistant/components/ring.py
Outdated
from ring_doorbell import Ring | ||
|
||
ring = Ring(username, password) | ||
if ring.is_connected: |
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.
You want to return False
if not connected, otherwise the component will be seen as set up correctly but the platforms will not be able to find the entry in hass.data
tests/components/test_ring.py
Outdated
|
||
def mocked_requests_get(*args, **kwargs): | ||
"""Mock requests.get invocations.""" | ||
class MockResponse: |
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.
We have exactly this functionality already available as requests_mock
. See this example: https://github.com/home-assistant/home-assistant/blob/dev/tests/util/test_location.py#L62-L66
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.
Thanks for this tip @balloob
@balloob code updated. Thank you! |
Looks good! 🐬 |
Description:
ring_doorbell
3rd party Python moduleExample entry for
configuration.yaml
(if applicable):HA Forum thread: https://community.home-assistant.io/t/ring-doorbell/7943
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2231
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][ex-requir]).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