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
Refactored units and icons for the Dyson sensors #14550
Conversation
@@ -14,12 +14,20 @@ | |||
DEPENDENCIES = ['dyson'] | |||
|
|||
SENSOR_UNITS = { | |||
'air_quality': 'level', | |||
'dust': 'level', | |||
'air_quality': '', |
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 assume None
is preferred instead of an empty 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.
Well spotted, I see other sensors having None
indeed as unit. Fixed by the next commit.
@CharlesBlonde, could you please take a look at this? 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.
Use super
to call the parent class init method.
@@ -43,12 +51,13 @@ def setup_platform(hass, config, add_devices, discovery_info=None): | |||
class DysonSensor(Entity): | |||
"""Representation of Dyson sensor.""" | |||
|
|||
def __init__(self, hass, device): | |||
def __init__(self, hass, device, 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.
Don't pass in hass
. It will be set on the entity when it has been added to home assistant.
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.
Ah, good to know. It's true that most of the sensors do not have hass
in the __init__()
signature (but a good fraction of them still has it...).
I'm happy to do the change, but just to note that both suggestions refer to the original code, not to this PR, and my attempt was to minimize the change.
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.
Since you're changing these lines, please clean it up.
|
||
class DysonFilterLifeSensor(DysonSensor): | ||
"""Representation of Dyson filter life sensor (in hours).""" | ||
|
||
def __init__(self, hass, device): | ||
"""Create a new Dyson filter life sensor.""" | ||
DysonSensor.__init__(self, hass, device) | ||
self._name = "{} filter life".format(self._device.name) | ||
DysonSensor.__init__(self, hass, device, 'filter_life') |
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__(device, 'filter_life')
Hi,
I'm quite busy right now but I'll try to take a look at this issue this
weekend !
Le jeu. 31 mai 2018 à 14:27, Martin Hjelmare <notifications@github.com> a
écrit :
… ***@***.**** commented on this pull request.
------------------------------
In homeassistant/components/sensor/dyson.py
<#14550 (comment)>
:
> @@ -43,12 +51,13 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
class DysonSensor(Entity):
"""Representation of Dyson sensor."""
- def __init__(self, hass, device):
+ def __init__(self, hass, device, sensor_type):
Since you're changing these lines, please clean it up.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14550 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQj8KHfMF5Q1eBlL7m0I_NA7txiERbnks5t3-HMgaJpZM4UF4lu>
.
|
Hi @CharlesBlonde, whenever you can please have a look at the latest version, which includes an overall refactoring. It works for my setup. |
For info, the CI test failed because in the test environment it seems that
Whereas in the real environment that works. @MartinHjelmare, what would be your suggestion to fix the tests? |
Simple way is to set |
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.
Looks good!
I'll wait for @CharlesBlonde or @fabaff to give the go ahead. |
@CharlesBlonde or @fabaff any further input? Otherwise I'll merge. |
As a side note, I didn't further push for this because lately the Dyson integration is broken for me for about a month now. Nothing related to Home Assistant, but rather the underlying Did not have time to debug it, but maybe @CharlesBlonde knows more and/or experienced the same? If so, that's obviously out of scope for this PR and we should follow it up with |
Description:
This PR bears a number of improvements for the Dyson sensors, to bring them to the HA standard:
The latter change is a breaking change.
Related issue (if applicable): N/A
Pull request in home-assistant.github.io with documentation (if applicable): N/A (though the sensors' names could be documented)
Example entry for
configuration.yaml
(if applicable): N/AChecklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices: