-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Airtouch4 integration #43513
Airtouch4 integration #43513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@cmroche thanks for reviewing this. Is there anything that I need to do to move this to the next stage or is it just a case of waiting? (sorry if that's a dumb question, first open source PR) |
Howdy everyone, any chance we can get this finalised soon? |
Hi @noobydp, any idea of who needs to be contacted to move this forward? I've been waiting for any feedback or someone to move it forward, but have heard nothing since last year |
Sorry I dont know, I'm just an innocent bystander :P |
@frenck sorry to tag you but you were really quick on the documentation PRs - are you able to give us any guidance on how to get this integration moved forward? Seems like it has been parked for months and I'm not really sure what to do. There's a bunch of people really keen for it, and we'd all like to work on adding improvements! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at your code and had the following remarks. Hopefully it is helpful.
@iMicknl thank you greatly for taking the time to look through this PR and give it a review - I have attempted to address all the things you raised (and have also merged in some minor tweaks to the behaviour/bug fixes requested by the community members who have been testing the integration) - let me know if there is anything else that needs changing :) |
@MartinHjelmare thanks very much for all your work reviewing this PR, really appreciate it - hopefully 3rd time's the charm! Please let me know if anything else is needed :) |
@LonePurpleWolf just an FYI the manifest.json is missing a version tag. In the lastest Home Assistant version this appear to prevent the integration from loading as a result. As a test I added a version tag into the manifest and restarted HA and this then allowed the integration to load correctly in the lastest version of HA |
@salty2011 do you have a reference for this? I could just be looking in the wrong place but I can't find any other integrations with a version tag - I know that custom integrations required a version tag https://developers.home-assistant.io/blog/2021/01/29/custom-integration-changes/ - if you're trying to use this integration by manually adding it as a custom integration that is outside the scope of this PR as discussed here https://community.home-assistant.io/t/airtouch-4-integration-aus/233295/76 (and is mostly a workaround for the fact this PR has been waiting 6 months) - if not, please let me know where I need to put a versions tag Thanks! |
This is a core integration addition, core integrations don't need a version tag. That comment is incorrect. |
@LonePurpleWolf @frenck ,apologies wasnt aware that core integration didn't require the tag. Just something I came across while trying to manually test the integration. Aside from that, appear to be working well |
@LonePurpleWolf Quick scan over this PR (sorry first time for me looking at this code), it looks pretty nice at first glance! So, let's get this train choo choo-ing 🚂 I think it is best if you'd rebased this PR onto the latest At that point, I think we can do some minor upgrades/changes that reduce the code a bit, since, after the rebase, this PR will have access to some nice helpers that allow for that. If you have any questions, please let me know, I'm happy to help. ../Frenck |
4038fc7
to
4dbe291
Compare
Hi @frenck thanks very much for taking a look! I have (hopefully) rebased onto more recent code, and also hoping that the builds might get a bit further this time (made a slight change to the external library). Let me know if theres things you think should be changed to use new helpers (not too sure what is available sorry) |
ceeb658
to
3fc82d4
Compare
@MartinHjelmare @frenck hoping I've addressed the stuff you guys raised in the last round of code review - thanks for all the suggestions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Proposed change
Adds integration with AirTouch 4 smart AC Controllers. This has been discussed in a few forum posts: https://community.home-assistant.io/t/airtouch-4-integration-aus/233295/ and https://community.home-assistant.io/t/polyaire-air-touch-2-or-3/19650/21
Type of change
Example entry for
configuration.yaml
:Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: