Improve Honeywell US climate component#5313
Conversation
|
@titilambert, thanks for your PR! By analyzing the history of the files in this pull request, we identified @turbokongen, @fabaff and @kk7ds to be potential reviewers. |
There was a problem hiding this comment.
line too long (92 > 79 characters)
There was a problem hiding this comment.
line too long (98 > 79 characters)
There was a problem hiding this comment.
line too long (81 > 79 characters)
There was a problem hiding this comment.
line too long (149 > 79 characters)
titilambert
force-pushed
the
honeywell
branch
from
5450f6d to
11685d3
Compare
There was a problem hiding this comment.
undefined name 'CONF_AWAY_TEMPERATURE'
There was a problem hiding this comment.
undefined name 'CONF_AWAY_TEMPERATURE'
There was a problem hiding this comment.
undefined name 'CONF_AWAY_TEMPERATURE'
There was a problem hiding this comment.
line too long (85 > 79 characters)
titilambert
force-pushed
the
honeywell
branch
from
654671c to
2ea4492
Compare
|
Will this work with the Honeywell Lyric thermostats? |
|
@arsaboo I don't think so, my thermostat is not a Lyric thermostat :/ |
titilambert
force-pushed
the
honeywell
branch
3 times, most recently
from
b621fa5 to
aedb06d
Compare
There was a problem hiding this comment.
redefinition of unused 'ATTR_FAN_MODE' from line 9
titilambert
force-pushed
the
honeywell
branch
10 times, most recently
from
ca7cafb to
39c5d4e
Compare
titilambert
changed the title
|
:/ Need to fix tests before |
titilambert
force-pushed
the
honeywell
branch
from
5005526 to
1261c54
Compare
There was a problem hiding this comment.
line too long (82 > 79 characters)
There was a problem hiding this comment.
line too long (82 > 79 characters)
There was a problem hiding this comment.
line too long (82 > 79 characters)
titilambert
force-pushed
the
honeywell
branch
from
1261c54 to
2270ae3
Compare
titilambert
force-pushed
the
honeywell
branch
from
2270ae3 to
c6a6d03
Compare
titilambert
force-pushed
the
honeywell
branch
from
c6a6d03 to
19b7dd2
Compare
titilambert
force-pushed
the
honeywell
branch
from
19b7dd2 to
f6f9501
Compare
titilambert
force-pushed
the
honeywell
branch
from
f6f9501 to
66469c9
Compare
|
Tests fixed ! |
| ) | ||
| except socket.error: | ||
| _LOGGER.error( | ||
| _LOGGER.exception( |
There was a problem hiding this comment.
We should not use .exception. This will cause a stacktrace to be printed for the users although the error message already tells them what is going on.
| client = somecomfort.SomeComfort(username, password) | ||
| except somecomfort.AuthError: | ||
| _LOGGER.error('Failed to login to honeywell account %s', username) | ||
| _LOGGER.exception('Failed to login to honeywell account %s', username) |
| return False | ||
| except somecomfort.SomeComfortError as ex: | ||
| _LOGGER.error('Failed to initialize honeywell client: %s', str(ex)) | ||
| _LOGGER.exception('Failed to initialize honeywell client: %s', str(ex)) |
| except StopIteration: | ||
| _LOGGER.error("Did not receive any temperature data from the " | ||
| "evohomeclient API.") | ||
| _LOGGER.exception("Did not receive any temperature data from the " |
There was a problem hiding this comment.
This whole method actually seems very weird. Why would temperatures throw a StopIteration error and how will we know that we have already assigned a value?
There was a problem hiding this comment.
Good questions... I don't know... the code was here 6 month ago (ada4de3#diff-896a11ee424d43689886053ccedb92d9R178) maybe @turbokongen can help us with this ?
There was a problem hiding this comment.
My bad, didn't realize it wasn't you. (still weird ;-) )
| temperature) | ||
| except somecomfort.SomeComfortError: | ||
| _LOGGER.error('Temperature %.1f out of range', temperature) | ||
| _LOGGER.exception('Temperature %.1f out of range', temperature) |
| def current_operation(self: ClimateDevice) -> str: | ||
| """Return current operation ie. heat, cool, idle.""" | ||
| return getattr(self._device, ATTR_SYSTEM_MODE, None) | ||
| oper = getattr(self._device, ATTR_CURRENT_OPERATION, None) |
There was a problem hiding this comment.
Will this do I/O inside the event loop? (which is not allowed)
There was a problem hiding this comment.
No, we need to run self._device.refresh() to update this value
There was a problem hiding this comment.
And I'm confirming it, using tcpdump
Good questions... I don't know... the code was here 6 month ago (ada4de3#diff-896a11ee424d43689886053ccedb92d9R178) maybe @turbokongen can help us with this ? |
|
Top! 🐬 |
10 participants
Description:
This patch import the US Honeywell component. It adds the
operation mode,fan modeandaway modeIt adds also some other attributes:
fan: return the current fan state (idle, orrunning)away_modereturn the current away mode state (trueorfalse)state: return the current operation mode (idle,off,heat,cool)Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2057
Checklist:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass