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

CoolMasterNet Climate platform #20787

Merged
merged 4 commits into from Feb 10, 2019

Conversation

@OnFreund
Copy link
Contributor

OnFreund commented Feb 6, 2019

Description:

CoolMasterNet as a Climate platform. This allows you to controller multiple HVAC instances connected to a single CoolMasterNet controller.
The integration is based on pycoolmaster.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

climate:
  - platform: coolmaster
    host: YOUR_COOLMASTER_HOST
    port: YOUR_COOLMASTER_PORT
    supported_modes:
      - heat
      - cool
      - dry

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.
@OnFreund

This comment has been minimized.

Copy link
Contributor Author

OnFreund commented Feb 6, 2019

Might be missing something, but it seems like the Travis failure isn't directly related to my changes:

3.5.3 is not installed; attempting download
Downloading archive: https://s3.amazonaws.com/travis-python-archives/binaries/ubuntu/14.04/x86_64/python-3.5.3.tar.bz2
$ curl -sSf -o python-3.5.3.tar.bz2 ${archive_url}
curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104
Unable to download 3.5.3 archive. The archive may not exist. Please consider a different version.
@OnFreund OnFreund force-pushed the OnFreund:coolmasternet_integration branch from 818ea25 to 788bba8 Feb 7, 2019
@OnFreund

This comment has been minimized.

Copy link
Contributor Author

OnFreund commented Feb 7, 2019

Another failure that doesn't seem related:

=================================== FAILURES ===================================
________ TestStatisticsSensor.test_initialize_from_database_with_maxage ________
@OnFreund OnFreund force-pushed the OnFreund:coolmasternet_integration branch from 788bba8 to f0c327a Feb 7, 2019
@OnFreund

This comment has been minimized.

Copy link
Contributor Author

OnFreund commented Feb 8, 2019

Hmm, not sure how to get someone to review this. I think this could be a helpful platform...

Copy link
Member

andrewsayre left a comment

Overall looks pretty good! Some additional comments:

  1. Do you want to add yourself to CODEOWNERS so you get tagged for future PRs over this code?
  2. Would you consider adding Unit Tests (and undo the .coveragerc change). Testing a platform is pretty easy and I see very little mocking needed. See the SmartThings component for some example platform tests.
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
OnFreund added 2 commits Feb 5, 2019
@OnFreund OnFreund force-pushed the OnFreund:coolmasternet_integration branch from f0c327a to 891edda Feb 9, 2019
@OnFreund

This comment has been minimized.

Copy link
Contributor Author

OnFreund commented Feb 9, 2019

@andrewsayre thanks so much.
I addressed all your comments (and also fixed the original wrong docstrings on the demo platform), and added myself as a code owner.

As for tests, since there's such little logic here, it felt like the added tax here for everyone else developing (the test suite takes long as it is, adding more tests makes it slower) is not worth it, but if you feel otherwise I'm happy to reconsider.

@andrewsayre andrewsayre self-assigned this Feb 9, 2019
@andrewsayre

This comment has been minimized.

Copy link
Member

andrewsayre commented Feb 9, 2019

As for tests, since there's such little logic here, it felt like the added tax here for everyone else developing (the test suite takes long as it is, adding more tests makes it slower) is not worth it, but if you feel otherwise I'm happy to reconsider.

I think there's always value in adding tests--I subscribe to the philosophy, If there aren't tests, it doesn't work. Certainly something to consider in the future!

CLA bot is stuck, but everything looks good. I'll approve. @MartinHjelmare would you take a quick pass given this is my first time reviewing a new platform? :)

Copy link
Member

MartinHjelmare left a comment

Generally looks good. Some adjustments needed.

.coveragerc Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
homeassistant/components/climate/coolmaster.py Outdated Show resolved Hide resolved
@OnFreund

This comment has been minimized.

Copy link
Contributor Author

OnFreund commented Feb 9, 2019

Thanks @MartinHjelmare. I addressed all your comments.

Copy link
Member

MartinHjelmare left a comment

Nice! Can be merged when last comment is addressed.

@OnFreund OnFreund force-pushed the OnFreund:coolmasternet_integration branch from 3f62075 to 36f9436 Feb 10, 2019
Copy link
Member

MartinHjelmare left a comment

Thanks!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 10, 2019

For the future, please don't squash commits after review has started to make it easier for readers to track changes. We will squash all the commits when merging.

@MartinHjelmare MartinHjelmare merged commit d8993af into home-assistant:dev Feb 10, 2019
5 checks passed
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 decreased (-0.01%) to 93.354%
Details
@ghost ghost removed the in progress label Feb 10, 2019
@OnFreund OnFreund deleted the OnFreund:coolmasternet_integration branch Feb 11, 2019
@balloob balloob mentioned this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.