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

Genius hub #21598

Merged
merged 55 commits into from
Apr 16, 2019
Merged

Genius hub #21598

merged 55 commits into from
Apr 16, 2019

Conversation

GeoffAtHome
Copy link
Contributor

@GeoffAtHome GeoffAtHome commented Mar 2, 2019

Description:

Added support to connect to Genius hub. This brings in climate controls, switches, TRV and room sensors.

Details about Genius hub can be found here: https://www.geniushub.co.uk/

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

Example entry for configuration.yaml (if applicable):

geniushub:
  username: !secret genius_username
  password: !secret genius_password
  host: ip_address

Checklist:

  • [yes ] The code change is tested and works locally.
  • [ yes] Local tests pass with tox. Your PR cannot be merged unless tests pass
  • [ yes] 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:

  • [yes] New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • [yes] New dependencies are only imported inside functions that use them ([example][ex-import]).
  • [yes] New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • [yes] 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.

@GeoffAtHome
Copy link
Contributor Author

@technicalpickles Help please. How to I move this forward? What do I need to do?

@frenck
Copy link
Member

frenck commented Mar 4, 2019

⚠️ I was unable to find a documentation PR (matching this code change) in our documentation repository.

Please, update the documentation and open a PR for it (be sure to create a documentation PR against the next branch) and link 🔗 this PR and the new documentation PR by mentioning the link to the PR's in both openings posts/descriptions.

🏷 I am adding the docs-missing label until this has been resolved.

@frenck
Copy link
Member

frenck commented Mar 4, 2019

Came across the documentation PR. PR's where not linked. Added links to OP's.
Removing docs-missing label.

homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Please strip it down to only include the component and the climate platform. More platforms can come later one platform and PR at a time. Smaller PRs are easier to review and will be merged faster.

See more comments below.

homeassistant/components/geniushub/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (dev@e834345). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev   #21598   +/-   ##
======================================
  Coverage       ?   93.82%           
======================================
  Files          ?      449           
  Lines          ?    36556           
  Branches       ?        0           
======================================
  Hits           ?    34299           
  Misses         ?     2257           
  Partials       ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e834345...d09dfa0. Read the comment docs.

Copy link
Member

@MartinHjelmare MartinHjelmare 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 a manifest.json file. See other integrations for examples.

homeassistant/components/geniushub/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Almost ready.

homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
homeassistant/components/geniushub/climate.py Outdated Show resolved Hide resolved
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Nice! Can be merged when build passes.

@zxdavb
Copy link
Contributor

zxdavb commented Apr 16, 2019

Nice! Can be merged when build passes.

So we can merge!

I think it looks a lot better after the refactor. 🥇

@MartinHjelmare MartinHjelmare merged commit e02a5f0 into home-assistant:dev Apr 16, 2019
@MartinHjelmare
Copy link
Member

MartinHjelmare commented Apr 16, 2019

Hmm, I'm reading the docs PR. Is it true that host config option doubles as option for api key besides host ip address? That's not a good way of structuring the config options. Each option should only have one type of meaning.

@zxdavb
Copy link
Contributor

zxdavb commented Apr 16, 2019

I wanted the client API to be agnostic of the two sources of data. The client API has this:

self._api_v1 = not (username or password)
if self._api_v1:  #else is v3
   ...

I really call it a Hub Identifier, the name host is left over from when Geoff originally started the PR.

I can change it if you want, although I guess it would have to be a fresh PR now?

Would you accept hub_id, or would you prefer:

geniushub:
  hub_token: skdjflksjfsljd...

... or ...

geniushub:
  host: 192.168.0.2
  username: my_username
  password: my password

But how do I do mutually exclusive keys in voluptuous?

@MartinHjelmare
Copy link
Member

Yes, please open a new PR with these changes. Use vol.Exclusive.

We have CONF_ACCESS_TOKEN and CONF_TOKEN in const.py. hub_token would be ok too.

@zxdavb
Copy link
Contributor

zxdavb commented Apr 16, 2019

OK, it's 11:30pm here - I will have it done by tomorrow.

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

Successfully merging this pull request may close these issues.

7 participants