-
-
Notifications
You must be signed in to change notification settings - Fork 563
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
Fix water_level
calculation for humidifiers
#1140
Conversation
@rytilahti Can you help me? Docstring CI fails on multiple files. Should I improve them all in this PR? |
@bieniu yeah, it's better to fix all affected files in cases like this. I'm not sure why it wasn't fixed automatically (and why the doc8 problem got in in the first place), but I just fixed it by running |
Codecov Report
@@ Coverage Diff @@
## master #1140 +/- ##
==========================================
+ Coverage 76.06% 76.10% +0.03%
==========================================
Files 76 76
Lines 8921 8926 +5
Branches 748 750 +2
==========================================
+ Hits 6786 6793 +7
+ Misses 1951 1950 -1
+ Partials 184 183 -1
Continue to review full report at Codecov.
|
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.
Do you mind adding some tests for the behavior, so that it won't break in the future?
miio/airhumidifier.py
Outdated
""" | ||
depth = self.data.get("depth") | ||
if depth is not None and depth <= 125: | ||
return int(depth / 1.25) | ||
return int(max(min(depth / 1.2, 100), 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.
Why use max(val, 0)
here, avoiding negative 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.
Because we have this
else if (value === -1) {
value = 0;
}
in the app source.
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.
Sorry for the delay, I see. I think it would be the best to inverse the condition and bail out early
if depth is None or depth < 0:
return None
return <real value 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.
I did simplify the logic a bit. If it looks okay to you, I think this is ready to be merged after the tests are passing.
Yes, I'll try to add some tests. |
@rytilahti Wow, you were busy today :) Great work! |
This PR fixes the calculation of the
water_level
according to the source of Mi Home app here and here.Fix: #1135