Skip to content
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

Add target_temp_high/low and current_temperature #21393

Merged
merged 2 commits into from Apr 16, 2019

Conversation

elupus
Copy link
Contributor

@elupus elupus commented Feb 24, 2019

water_heater piggy back on climate(thermostat) component in gui,
so these things are already supported by frontend for display
purposes. (and sort of already works by using device_state_attributes)

Description:

This is an attempt to make the water_heater component a bit more usable without really touching
on the architectural issues with it.

Related issue (if applicable): partial #19093

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@elupus
Copy link
Contributor Author

elupus commented Feb 24, 2019

The coverage test failure doesn't seem related

@elupus elupus closed this Feb 24, 2019
@ghost ghost removed the in progress label Feb 24, 2019
@elupus elupus reopened this Feb 24, 2019
@ghost ghost added the in progress label Feb 24, 2019

ATTR_MAX_TEMP = 'max_temp'
ATTR_MIN_TEMP = 'min_temp'
ATTR_AWAY_MODE = 'away_mode'
ATTR_OPERATION_MODE = 'operation_mode'
ATTR_OPERATION_LIST = 'operation_list'
ATTR_TARGET_TEMP_HIGH = 'target_temp_high'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't do this. If a water heater is set to auto, we should just assume it supports a high/low temp.

We should not copy the mistakes from the climate component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure what you mean here. Maybe comment is on wrong line. You mean the support features? the attributes are needed to be able to display. But the SUPPORT_TARGET_* are probably pointless. If the AUTO mode have no target min/max, they should just return None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@balloob can you clarify?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, this should be on the support ones.

water_heater piggy back on climate(thermostat) component in gui,
so these things are already supported by frontend for display
purposes.
@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

Merging #21393 into dev will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #21393      +/-   ##
==========================================
- Coverage   94.15%   94.09%   -0.06%     
==========================================
  Files         452      452              
  Lines       36809    36797      -12     
==========================================
- Hits        34658    34625      -33     
- Misses       2151     2172      +21
Impacted Files Coverage Δ
homeassistant/components/hassio/ingress.py 67.76% <0%> (-10.29%) ⬇️
homeassistant/components/axis/device.py 93.18% <0%> (-6.82%) ⬇️
homeassistant/loader.py 93.15% <0%> (-0.69%) ⬇️
homeassistant/components/axis/camera.py 90.24% <0%> (-0.67%) ⬇️
...ssistant/components/google_assistant/smart_home.py 93.1% <0%> (-0.23%) ⬇️
homeassistant/components/hue/bridge.py 73.07% <0%> (ø) ⬆️
setup.py 86.41% <0%> (ø) ⬆️
homeassistant/components/google_assistant/const.py 100% <0%> (ø) ⬆️
homeassistant/components/http/view.py 96.59% <0%> (ø) ⬆️
homeassistant/bootstrap.py 59.25% <0%> (+0.23%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7102ea...789288c. Read the comment docs.

@balloob balloob merged commit 4ed1d9b into home-assistant:dev Apr 16, 2019
@elupus elupus deleted the water_heater_current branch May 2, 2019 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants