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 config flow for Daikin #19182

Merged
merged 5 commits into from Dec 16, 2018

Conversation

Projects
None yet
4 participants
@fredrike
Copy link
Contributor

fredrike commented Dec 11, 2018

Description:

Adds config-flow for the Daikin AC platform

Breaking change

Removes configuration option: monitored_conditions. Also removes configuration settings for sensor.daikin.

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 user exposed functionality or configuration variables are added/changed:

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 11, 2018

@MartinHjelmare you don't get rid of me that easy 😃. I'll add the tests later today if the rest looks good (I've (re)used most of tricks from tellduslive so I hope there is not that much sins in this).

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 11, 2018

Did you try moving the Daikin component module to the package module with git mv in a separate commit? Now it looks like we removed the old file and created a new file. It would be good if we kept history.

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 11, 2018

Did you try moving the Daikin component module to the package module with git mv in a separate commit? Now it looks like we removed the old file and created a new file. It would be good if we kept history.

Yes I did, but it looks like github doesn't see that. 0a5bec9

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 11, 2018

Can you try making the move in a separate commit with no other changes.

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 11, 2018

Can you try making the move in a separate commit with no other changes.

0a5bec9 is a separate commit, I could extract it to a separate PR if you want.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 11, 2018

It's one commit with multiple files changed. I'd like you to try to only move that file in one commit without any other changes in the commit.

If that doesn't fix the history, yes then I think we should do it in a separate PR.

@fredrike fredrike force-pushed the fredrike:daikin-registry branch from 8a292dc to 0c782a8 Dec 11, 2018

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 11, 2018

Still no difference, I'll create a PR with just the rename then..

@fredrike fredrike referenced this pull request Dec 11, 2018

Merged

Move daikin to package #19187

@fredrike fredrike force-pushed the fredrike:daikin-registry branch from 0c782a8 to 0d39eb2 Dec 11, 2018

@fabaff fabaff changed the title config-flow for daikin A config flow for Daikin Dec 12, 2018

@fabaff fabaff changed the title A config flow for Daikin Add config flow for Daikin Dec 12, 2018

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 14, 2018

You can do all the right Git commands and GitHub will still show it incorrectly 🤷‍♂️

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 14, 2018

This looks good. I just miss the tests for the config flow.

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

Make sure to check that all new python modules are excluded from coverage except the config flow that we should test.

Show resolved Hide resolved homeassistant/components/daikin/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/daikin/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/daikin/__init__.py
Show resolved Hide resolved homeassistant/components/daikin/config_flow.py Outdated
Show resolved Hide resolved homeassistant/components/daikin/config_flow.py Outdated

@fredrike fredrike force-pushed the fredrike:daikin-registry branch from c639473 to 0802c2f Dec 15, 2018

Show resolved Hide resolved tests/components/daikin/test_config_flow.py Outdated
Show resolved Hide resolved tests/components/daikin/test_config_flow.py Outdated
Show resolved Hide resolved tests/components/daikin/test_config_flow.py Outdated
Show resolved Hide resolved tests/components/daikin/test_config_flow.py Outdated
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 15, 2018

Also add a paragraph describing the breaking change in the PR description so we can copy that for the release notes.

@fredrike

This comment has been minimized.

Copy link
Contributor

fredrike commented Dec 16, 2018

pytest --cov=homeassistant/components/daikin/  --cov-report term-missing tests/components/daikin/test_config_flow.py
Test session starts (platform: darwin, Python 3.7.0, pytest 4.0.1, pytest-sugar 0.9.2)
rootdir: /Users/fer/Documents/github/home-assistant, inifile: setup.cfg
plugins: requests-mock-1.5.2, timeout-1.3.3, sugar-0.9.2, cov-2.6.0, aiohttp-0.3.0, pylama-7.6.5
collecting ...
 tests/components/daikin/test_config_flow.py ✓✓✓✓✓✓                                                                                                                                        100% ██████████

---------- coverage: platform darwin, python 3.7.0-final-0 -----------
Name                                             Stmts   Miss  Cover   Missing
------------------------------------------------------------------------------
homeassistant/components/daikin/config_flow.py      40      0   100%


Results (0.48s):
       6 passed

@fredrike fredrike referenced this pull request Dec 16, 2018

Merged

Update Daikin description #7872

2 of 2 tasks complete
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Awesome!

@MartinHjelmare MartinHjelmare merged commit 5a295ad into home-assistant:dev Dec 16, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 93.018%
Details

@wafflebot wafflebot bot removed the in progress label Dec 16, 2018

dshokouhi added a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018

Add config flow for Daikin (home-assistant#19182)
* config flow for daikin

* tox test

* return fixes

* tox test fixes

* tox formatting

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment