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 airtouch5 #98136

Merged
merged 31 commits into from Jan 23, 2024
Merged

Add airtouch5 #98136

merged 31 commits into from Jan 23, 2024

Conversation

danzel
Copy link
Contributor

@danzel danzel commented Aug 9, 2023

Proposed change

Add integration for the Airtouch 5 AC/Zone controller.
This has been used/tested by multiple people in the forum
The airtouch5py library is over here

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

home-assistant bot commented Aug 9, 2023

Hi @danzel

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!

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

@home-assistant home-assistant bot marked this pull request as draft August 9, 2023 22:46
@home-assistant
Copy link

home-assistant bot commented Aug 9, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@danzel
Copy link
Contributor Author

danzel commented Aug 9, 2023

How essential is "When adding new integrations, limit included platforms to a single platform. Please reduce this PR to a single platform"?

I can remove them if needed, and then submit them after this is merged. Would rather just do 1 PR though.
(I'd keep the climate entity, that is the most important one)

@danzel danzel marked this pull request as ready for review August 9, 2023 22:53
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

I can remove them if needed, and then submit them after this is merged. Would rather just do 1 PR though.
(I'd keep the climate entity, that is the most important one)

Please remove all except one. This makes the PR smaller and it will be easier to review. Therefore the PR will be merged faster.

Can you please include a link to the third-party library repository? It will help the review :)

@home-assistant home-assistant bot marked this pull request as draft August 18, 2023 08:55
@danzel danzel marked this pull request as ready for review August 19, 2023 01:04
@danzel
Copy link
Contributor Author

danzel commented Aug 19, 2023

@edenhaus Removed all except climate, linked to the library in the initial post.
Thanks

Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

I have done an initial review and have some feedback/questions.

.coveragerc Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/climate.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/entity.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/entity.py Outdated Show resolved Hide resolved
homeassistant/generated/config_flows.py Show resolved Hide resolved
homeassistant/components/airtouch5/climate.py Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft August 21, 2023 07:53
@danzel
Copy link
Contributor Author

danzel commented Aug 21, 2023

Have fixed what I can from the comments and rebased to fix build issues.

@Kaldek
Copy link

Kaldek commented Oct 5, 2023

Hi folks, can we please bump this one up in the queue please, it's been dragging on for a long time. I know we're only Australia and a tiny portion of the market, yada yada, but it has been a while.

@edenhaus
Copy link
Contributor

A PR in draft will be considered still in progress and not ready for another review. Therefore, we give less attention to drafted PRs.
@danzel Please mark the PR ready for review by clicking "Ready for review" after implementing the requested changes.

Sorry but I have to ask you again to fix the merge conflicts first

Change ac name to match the previous hack integration
Check changes before trying to apply them
New dep version to fix _id not wrapping around
@danzel danzel marked this pull request as ready for review November 15, 2023 08:09
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Some small cleanups, and then we are ready to merge :)

homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft November 15, 2023 09:06
@danzel
Copy link
Contributor Author

danzel commented Nov 20, 2023

Some small cleanups, and then we are ready to merge :)

Done, thanks for all the reviews

@danzel danzel marked this pull request as ready for review November 20, 2023 06:38
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Please add tests until CI is passing.
We require 100% test coverage of of the config flow and for all files not excluded by .coveragerc

@home-assistant home-assistant bot marked this pull request as draft November 20, 2023 13:09
@bolistik
Copy link

bolistik commented Dec 5, 2023

@danzel are you please able to complete the final checks on this one?

@danzel
Copy link
Contributor Author

danzel commented Dec 5, 2023 via email

@danzel danzel marked this pull request as ready for review December 19, 2023 02:03
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

There are some small simplifications. Otherwise, the PR is good to be merged :)

Please fix also the merge conflicts

homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/airtouch5/config_flow.py Outdated Show resolved Hide resolved
tests/components/airtouch5/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/airtouch5/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/airtouch5/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/airtouch5/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/airtouch5/test_config_flow.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft December 28, 2023 12:59
@danzel danzel marked this pull request as ready for review January 14, 2024 23:03
@danzel
Copy link
Contributor Author

danzel commented Jan 14, 2024

Changes applied, I needed to disable a warning to allow except Exception:, not sure why it wasn't required before.

Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Thanks @danzel 👍

@edenhaus
Copy link
Contributor

@danzel Can you please implement the suggested changes in the docs PR?

@emontnemery emontnemery reopened this Jan 23, 2024
@emontnemery
Copy link
Contributor

(Close + reopen to re-run CI)

@edenhaus edenhaus merged commit e3a73c1 into home-assistant:dev Jan 23, 2024
98 of 99 checks passed
@danzel
Copy link
Contributor Author

danzel commented Jan 23, 2024

@danzel Can you please implement the suggested changes in the docs PR?

Sorry, I missed those, looks like you've applied them, thanks.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
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.

None yet

6 participants