-
-
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
Device support for the Xiaomi Mi Smart Fan #52
Conversation
self.natural_level, self.speed_level, self.oscillate, | ||
self.battery, self.ac_power, self.poweroff_time, | ||
self.speed_level, self.angle) | ||
return s |
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 unsure if it makes sense to expose everything in __str__
for all devices, but that can be cleaned up for all supported devices at some point if needed.
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.
Alright.
mirobo/fan.py
Outdated
@property | ||
def temperature(self) -> float: | ||
if self.data["temp_dec"] is not None: | ||
return self.data["temp_dec"] / 10.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.
This requires return None in the end to avoid missing "return" (typing, flake8 etc. can be run by simply calling tox
in the source dir btw).
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.
My IDE is unhappy now because None isn't "float". ;-)
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, it should be defined as Optional[float]
then :-)
mirobo/fan.py
Outdated
@property | ||
def led_brightness(self) -> LedBrightness: | ||
if self.data["led_b"] is not None: | ||
return LedBrightness(self.data["led_b"]) |
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.
Ditto here, just add return None
to the end.
Looks much better now, thanks! Just a couple of nits to be done before merging. I'll probably have to look into how the output of mypy/flake8 can be integrated here directly to checks. |
Could you just tell me the commands I can execute locally to check? |
Executing |
mirobo/fan.py
Outdated
if self.data["temp_dec"] is not None: | ||
return self.data["temp_dec"] / 10.0 | ||
else: | ||
return None |
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 it's better to skip the else
block completely and just return False if the if
doesn't get hit.
1 similar comment
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!
Retry.