-
-
Notifications
You must be signed in to change notification settings - Fork 566
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
New properties of the Xiaomi Air Humidifier added #173
New properties of the Xiaomi Air Humidifier added #173
Conversation
miio/tests/test_airhumidifier.py
Outdated
|
||
self.device.set_child_lock(False) | ||
assert child_lock() is False | ||
|
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 at end of file
miio/tests/test_airhumidifier.py
Outdated
@@ -64,6 +69,9 @@ def test_status(self): | |||
assert self.state().mode == OperationMode(self.device.start_state["mode"]) | |||
assert self.state().led_brightness == LedBrightness(self.device.start_state["led_b"]) | |||
assert self.state().buzzer == (self.device.start_state["buzzer"] == 'on') | |||
assert self.state().child_lock == (self.device.start_state["child_lock"] == 'on') | |||
assert self.state().target_humidity == self.device.start_state["limit_hum"] | |||
assert self.state().favorite_level == self.device.start_state["trans_level"] |
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.
line too long (84 > 79 characters)
@@ -64,6 +69,9 @@ def test_status(self): | |||
assert self.state().mode == OperationMode(self.device.start_state["mode"]) | |||
assert self.state().led_brightness == LedBrightness(self.device.start_state["led_b"]) | |||
assert self.state().buzzer == (self.device.start_state["buzzer"] == 'on') | |||
assert self.state().child_lock == (self.device.start_state["child_lock"] == 'on') | |||
assert self.state().target_humidity == self.device.start_state["limit_hum"] |
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.
line too long (83 > 79 characters)
@@ -64,6 +69,9 @@ def test_status(self): | |||
assert self.state().mode == OperationMode(self.device.start_state["mode"]) | |||
assert self.state().led_brightness == LedBrightness(self.device.start_state["led_b"]) | |||
assert self.state().buzzer == (self.device.start_state["buzzer"] == 'on') | |||
assert self.state().child_lock == (self.device.start_state["child_lock"] == 'on') |
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.
line too long (89 > 79 characters)
@@ -1,6 +1,6 @@ | |||
from unittest import TestCase | |||
from miio import AirHumidifier | |||
from miio.airhumidifier import OperationMode, LedBrightness | |||
from miio.airhumidifier import OperationMode, LedBrightness, AirHumidifierException |
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.
line too long (83 > 79 characters)
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 think houndci should be configured to allow lines up to 100 lines, even when <80 is mostly preferred. This is not a biggie (esp. as it is in the tests), but can be fixed at some point.
assert child_lock() is True | ||
|
||
self.device.set_child_lock(False) | ||
assert child_lock() is False |
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.
no newline at end of file
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.
It's a good idea to have newlines at the end (for grepping etc.): https://stackoverflow.com/a/729795
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 swear there is a newline. I don't know what's going on here.
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 fine to me, I added a couple of improvement ideas.
miio/airhumidifier.py
Outdated
@@ -7,6 +7,10 @@ | |||
_LOGGER = logging.getLogger(__name__) | |||
|
|||
|
|||
class AirHumidifierException(Exception): |
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 should derive from DeviceException
(looks like we have some other exceptions too, which do not do this currently) to allow easy catching of all exception from this library no matter which device is being used.
miio/airhumidifier.py
Outdated
def set_favorite_level(self, level: int): | ||
"""Set favorite level.""" | ||
if level < 30 or level > 85: | ||
raise AirHumidifierException("Invalid favorite level: %s" % level) |
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.
Is there a link between this level and target humidity (which allows only specific values)?
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.
No. It's more like a "fan speed".
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'm waiting for some details currently: https://community.home-assistant.io/t/xiaomi-humidifier-support/35672/26
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.
Ok, feel free to merge when you are ready 👍
@@ -1,6 +1,6 @@ | |||
from unittest import TestCase | |||
from miio import AirHumidifier | |||
from miio.airhumidifier import OperationMode, LedBrightness | |||
from miio.airhumidifier import OperationMode, LedBrightness, AirHumidifierException |
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 think houndci should be configured to allow lines up to 100 lines, even when <80 is mostly preferred. This is not a biggie (esp. as it is in the tests), but can be fixed at some point.
assert child_lock() is True | ||
|
||
self.device.set_child_lock(False) | ||
assert child_lock() is False |
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.
It's a good idea to have newlines at the end (for grepping etc.): https://stackoverflow.com/a/729795
My last commit doesn't work. Right? |
Hmm, I'm not sure if it's working or not, but nevertheless it's a good idea to have those there. Btw, if it's not too much effort, could you cherrypick 000d06e and 02e1e53 off from this PR and just commit them separately? Then those are visible in commit logs making it easier to remember to add a note about them into the changelog. Otherwise feel free to commit when you think it's ready 👍 |
@@ -64,6 +69,9 @@ def test_status(self): | |||
assert self.state().mode == OperationMode(self.device.start_state["mode"]) | |||
assert self.state().led_brightness == LedBrightness(self.device.start_state["led_b"]) | |||
assert self.state().buzzer == (self.device.start_state["buzzer"] == 'on') | |||
assert self.state().child_lock == (self.device.start_state["child_lock"] == 'on') | |||
assert self.state().target_humidity == self.device.start_state["limit_hum"] | |||
assert self.state().trans_level == self.device.start_state["trans_level"] |
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.
line too long (81 > 79 characters)
Btw, instead of just reverting you can do a rebase, and simply drop that commit :-) (it will require a force push though..) |
No description provided.