Skip to content

Add Atag One thermostat integration#32361

Merged
MartinHjelmare merged 24 commits into
home-assistant:devfrom
MatsNl:atag
Apr 22, 2020
Merged

Add Atag One thermostat integration#32361
MartinHjelmare merged 24 commits into
home-assistant:devfrom
MatsNl:atag

Conversation

@MatsNl
Copy link
Copy Markdown
Contributor

@MatsNl MatsNl commented Feb 29, 2020

Description:

adds support for atag one thermostats: https://www.atag-one.com/
includes climate, waterheater and some basic sensors (e.g. outside temperature, burner intensity, etc)
still to do: develop code for hold modes

**Reopened pull request to fix some merge conflicts: #30796

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#11841

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.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Feb 29, 2020

@springstan reopened as discussed, can you let me know if anything needs to be done still?

Many thanks for your support already! 👍

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 29, 2020

Codecov Report

Merging #32361 into dev will decrease coverage by 0.17%.
The diff coverage is 22.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #32361      +/-   ##
==========================================
- Coverage   94.75%   94.57%   -0.18%     
==========================================
  Files         775      779       +4     
  Lines       56154    56309     +155     
==========================================
+ Hits        53209    53256      +47     
- Misses       2945     3053     +108
Impacted Files Coverage Δ
homeassistant/generated/config_flows.py 100% <ø> (ø) ⬆️
homeassistant/components/atag/sensor.py 0% <0%> (ø)
homeassistant/components/atag/climate.py 0% <0%> (ø)
homeassistant/components/atag/water_heater.py 0% <0%> (ø)
homeassistant/components/atag/config_flow.py 100% <100%> (ø)
homeassistant/components/light/device_action.py 94% <0%> (+0.06%) ⬆️

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 e9a7b66...a142907. Read the comment docs.

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Mar 1, 2020

@springstan I didn't exclude climate, sensor and water_heater files (as per the previous PR). Should they be, given the codecov report?

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Mar 2, 2020

@springstan would you mind helping out? codecov is throwing funny errors like codecov: error: argument --token/-t: expected one argument

I don't really know how to move on / get this merged anymore

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Mar 9, 2020

@springstan can you let me know the next step please?

@reharmsen
Copy link
Copy Markdown

Any news on whether and when this will be merged to core?

@frenck
Copy link
Copy Markdown
Member

frenck commented Mar 26, 2020

@reharmsen When it is done. Sorry, but we don't have a sensible answer to that question. Feel free to help us by contributing reviews to open PRs. 👍

@bdraco bdraco self-assigned this Apr 5, 2020
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 5, 2020

Slimmed down version of #30796

Comment thread homeassistant/components/atag/.translations/en.json Outdated
Comment thread homeassistant/components/atag/__init__.py Outdated
Comment thread homeassistant/components/atag/__init__.py Outdated
Comment thread homeassistant/components/atag/strings.json Outdated
Comment thread tests/components/atag/test_atag_flow.py Outdated
Comment thread homeassistant/components/atag/config_flow.py Outdated
Comment thread homeassistant/components/atag/config_flow.py Outdated
Comment thread homeassistant/components/atag/__init__.py Outdated
Comment thread homeassistant/components/atag/__init__.py Outdated
Comment thread homeassistant/components/atag/__init__.py Outdated
Comment thread homeassistant/components/atag/__init__.py Outdated
Comment thread homeassistant/components/atag/__init__.py Outdated
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

@MatsNl Thanks for the PR. Please rebase this to make sure its running the latest checks

https://developers.home-assistant.io/docs/development_catching_up/

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Apr 11, 2020

thanks for the review @bdraco! will get to it in the next week hopefully :)

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 17, 2020

@MatsNl There are files missing coverage that needs to be added or listed in .coveragerc

# pytest --cov=homeassistant/components/atag/ --cov-report term-missing -- tests/components/atag/
Test session starts (platform: linux, Python 3.8.1, pytest 5.3.5, pytest-sugar 0.9.2)
rootdir: /root/home-assistant, inifile: setup.cfg
plugins: requests-mock-1.7.0, aiohttp-0.3.0, cov-2.8.1, sugar-0.9.2, timeout-1.3.3, profiling-1.7.0
collecting ... 
 tests/components/atag/test_atag_flow.py ✓✓✓✓                                                                                                                   100% ██████████

----------- coverage: platform linux, python 3.8.1-final-0 -----------
Name                                            Stmts   Miss  Cover   Missing
-----------------------------------------------------------------------------
homeassistant/components/atag/__init__.py          93     42    55%   126, 131-148, 156-158, 164-170, 175-185, 193-199, 204-206, 217, 222-225, 230, 235, 240, 245, 250, 254, 260
homeassistant/components/atag/climate.py           72     72     0%   2-132
homeassistant/components/atag/config_flow.py       31      0   100%
homeassistant/components/atag/sensor.py            12     12     0%   2-22
homeassistant/components/atag/water_heater.py      38     38     0%   2-70
-----------------------------------------------------------------------------
TOTAL                                             246    164    33%


Results (0.79s):
       4 passed
root@ha-dev:~/home-assistant# 

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Apr 17, 2020

Are you sure? Because I was told earlier (in the preceding PR) that sensor and climate etc get excluded automatically. And in the codecov I actually see very different integrations listed as the source of the problem. happy to add those to the coveragerc ofcourse if you think that is the solution..?

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Apr 17, 2020

Are you sure? Because I was told earlier (in the preceding PR) that sensor and climate etc get excluded automatically. And in the codecov I actually see very different integrations listed as the source of the problem. happy to add those to the coveragerc ofcourse if you think that is the solution..?

I didn't personally setup the coverage checks so I can't know for sure but I do see ~ 218 integrations listing sensor.py and ~ 27 listing climate.py in .coveragerc

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Apr 17, 2020

interesting - ok ill push a fix shortly

@bdraco bdraco self-requested a review April 19, 2020 19:28
Comment thread homeassistant/components/atag/__init__.py Outdated
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM

Co-Authored-By: J. Nick Koston <nick@koston.org>
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good! Some comments.

Comment thread homeassistant/components/atag/climate.py Outdated
Comment thread homeassistant/components/atag/climate.py Outdated
Comment thread homeassistant/components/atag/climate.py Outdated
Comment thread homeassistant/components/atag/climate.py Outdated
Comment thread homeassistant/components/atag/climate.py Outdated
Comment thread tests/components/atag/test_atag_flow.py Outdated
Comment thread tests/components/atag/test_atag_flow.py Outdated
Comment thread tests/components/atag/test_atag_flow.py Outdated
Comment thread tests/components/atag/test_atag_flow.py Outdated
Comment thread tests/components/atag/test_atag_flow.py Outdated
MatsNl and others added 2 commits April 20, 2020 18:31
Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>
@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Apr 20, 2020

Thanks for the review @MartinHjelmare! I pushed the necessary fixes, only thing I'm unsure about is whether the fake off mode is acceptable ? Given that the device doesn't have that functionality and it can be quite useful to disable directly on the climate entity.

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Apr 20, 2020

I think the flake8 tests on azure differ from the dev container, pre-commit passed in my environment?

Comment thread homeassistant/components/atag/climate.py
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks great!

@MartinHjelmare
Copy link
Copy Markdown
Member

Just one clean up needed: Please rename the config flow test module to test_config_flow.py.

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Apr 22, 2020

Done! Hope this doesn't result in caching errors on azure, thats why I had changed the name earlier

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MatsNl
Copy link
Copy Markdown
Contributor Author

MatsNl commented Apr 22, 2020

Cool! Thanks everyone for the reviews & support!

@lock lock Bot locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants