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 new integration for Jandy iAqualink pool control #26034

Merged
merged 4 commits into from Sep 6, 2019

Conversation

@flz
Copy link
Contributor

commented Aug 18, 2019

Description:

This is a new component introducing support for Jandy iAqualink pool controls.

This PR includes support for the climate platform only.

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

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 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.
@flz flz requested review from Adminiuga, andrewsayre, awarecan, bachya, dmulcahey, Jc2k, Kane610, zxdavb and home-assistant/core as code owners Aug 18, 2019
@flz flz requested a review from Aug 18, 2019
@flz flz requested a review from home-assistant/z-wave as a code owner Aug 18, 2019
@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

Hi @flz,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@project-bot project-bot bot added this to Needs review in Dev Aug 18, 2019
@flz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

Ugh. This shouldn't try to push 56 commits.

@homeassistant homeassistant added cla-signed and removed cla-needed labels Aug 18, 2019
@flz flz force-pushed the flz:iaqualink branch from 60af65c to b8c71fc Aug 18, 2019
@MartinHjelmare MartinHjelmare removed request for home-assistant/core, Jc2k, bachya, dmulcahey, Adminiuga, andrewsayre, zxdavb, awarecan and Kane610 Aug 18, 2019
@flz flz force-pushed the flz:iaqualink branch from b8c71fc to 3f513e7 Aug 18, 2019
@flz flz changed the title Add new integrations for Jandy iAqualink pool control Add new integration for Jandy iAqualink pool control Aug 18, 2019
@flz flz referenced this pull request Aug 18, 2019
2 of 2 tasks complete
Copy link
Member

left a comment

Thanks for the PR. Please start by removing the light, sensor and switch platforms from this PR. More platforms can come in later PRs, one platform per PR. We want PRs as small as possible for quicker merge.

homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/manifest.json Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Aug 31, 2019
@flz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Thanks for the review.

Please update the documentation to reflect that new integrations shouldn't contain more than a single platform for the reasons you mention. I understand the goal of making PR somewhat self-contained but in the case of a new integration, this seems rather arbitrary, especially when undocumented.

@andrewsayre

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

Please update the documentation to reflect that new integrations shouldn't contain more than a single platform for the reasons you mention.

The docs do indicate that, but please let us know how we could make it more clear -- or better yet, submit a PR to the docs themselves with your improvements. :)

@flz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2019

Apologies, the documentation is clear. I obviously have missed it!

@flz flz force-pushed the flz:iaqualink branch 2 times, most recently from 46cb3db to cf5ff34 Sep 2, 2019
@flz flz force-pushed the flz:iaqualink branch from cf5ff34 to 8d69d9d Sep 2, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Please don't squash commits after review has started to make it easier for readers to track changes.

@flz flz changed the title Add new integration for Jandy iAqualink pool control [1/4] Add new integration for Jandy iAqualink pool control with climate platform Sep 2, 2019
@flz flz changed the title [1/4] Add new integration for Jandy iAqualink pool control with climate platform Add new integration for Jandy iAqualink pool control Sep 2, 2019
homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/climate.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/climate.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/climate.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/climate.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/climate.py Outdated Show resolved Hide resolved
homeassistant/components/iaqualink/config_flow.py Outdated Show resolved Hide resolved
tests/components/iaqualink/test_config_flow.py Outdated Show resolved Hide resolved
flz added 2 commits Sep 3, 2019
Copy link
Member

left a comment

Looks good!

Dev automation moved this from Review in progress to Reviewer approved Sep 3, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Can be merged when build passes.

@flz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Neat, thanks again for the help!

@MartinHjelmare MartinHjelmare merged commit 0abb2f3 into home-assistant:dev Sep 6, 2019
11 checks passed
11 checks passed
CI Build #20190903.84 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 93.9%)
Details
codecov/project 93.99% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 6, 2019
@lock lock bot locked and limited conversation to collaborators Sep 7, 2019
@flz flz deleted the flz:iaqualink branch Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.