Improve configuration schema for Geniushub integration by zxdavb · Pull Request #23155 · home-assistant/core

zxdavb · 2019-04-17T08:26:48Z

Breaking Change:

Nil, original PR, #21598, is still in dev

Description:

See config section, below.

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

Example entry for `configuration.yaml` (if applicable):

As per @MartinHjelmare request, config is now be either of:

geniushub:
  token: skdjflksjfsljd...

... or ...

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

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:

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

N/A The manifest file has all fields filled out correctly (example).
N/A New dependencies have been added to requirements in the manifest (example).
N/A New dependencies are only imported inside functions that use them (example).
N/A 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:

N/A - Tests have been added to verify that the new code works.

…dentials

MartinHjelmare · 2019-04-17T16:48:23Z

+        username = hass_config[DOMAIN].get(CONF_USERNAME)
+        password = hass_config[DOMAIN].get(CONF_PASSWORD)
+    else:
+        host = hass_config[DOMAIN].get(CONF_TOKEN)


Don't call this host, when it's not a host.

Eg collect the arguments in a tuple and pass the tuple to the class.

Which do you prefer:

host = token = None if CONF_HOST in hass_config[DOMAIN]: host = hass_config[DOMAIN].get(CONF_HOST) username = hass_config[DOMAIN].get(CONF_USERNAME) password = hass_config[DOMAIN].get(CONF_PASSWORD) else: token = hass_config[DOMAIN].get(CONF_TOKEN) username = password = None try: client = geniushub_data['client'] = GeniusHubClient( host if host else token, username, password, session=async_get_clientsession(hass) )

or

if CONF_HOST in hass_config[DOMAIN]: hub_identifier = hass_config[DOMAIN].get(CONF_HOST) username = hass_config[DOMAIN].get(CONF_USERNAME) password = hass_config[DOMAIN].get(CONF_PASSWORD) else: hub_identifier = hass_config[DOMAIN].get(CONF_TOKEN) username = password = None try: client = geniushub_data['client'] = GeniusHubClient( hub_identifier, username, password, session=async_get_clientsession(hass) )

or

host = hass_config[DOMAIN].get(CONF_HOST) username = hass_config[DOMAIN].get(CONF_USERNAME) password = hass_config[DOMAIN].get(CONF_PASSWORD) token = hass_config[DOMAIN].get(CONF_TOKEN) try: client = geniushub_data['client'] = GeniusHubClient( host if host else token, username, password, session=async_get_clientsession(hass) )

Collect the arguments in a tuple.

if blabla: args = arg1, arg2, etc else: args = (arg4, ) client = class(*args, session=session)

Oh, I see - but it would a tuple of 1 (hub_id), the rest are kwargs/dict. I don't see what would be gained:

def __init__(self, hub_id, username=None, password=None, session=None, debug=False)

So how about:

kwargs = dict(hass_config[DOMAIN]) if CONF_HOST in kwargs: args = (kwargs.pop(CONF_HOST), ) else: args = (kwargs.pop(CONF_TOKEN), ) try: client = geniushub_data['client'] = GeniusHubClient( *args, **kwargs, session=async_get_clientsession(hass) )

We gain that we don't confuse ourselves by naming a variable something which it isn't.

👍 In addition, I think it's kind of elegant - you get a two-for-one from voluptuous.

MartinHjelmare

Can be merged when last comment is addressed.

…oken

codecov · 2019-04-18T01:00:04Z

Codecov Report

Merging #23155 into dev will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev   #23155      +/-   ##
==========================================
+ Coverage   94.19%    94.2%   +<.01%     
==========================================
  Files         453      453              
  Lines       36915    36915              
==========================================
+ Hits        34773    34775       +2     
+ Misses       2142     2140       -2

Impacted Files	Coverage Δ
homeassistant/components/uk_transport/sensor.py	`93.43% <0%> (-0.73%)`	⬇️
...ssistant/components/islamic_prayer_times/sensor.py	`97.87% <0%> (+3.19%)`	⬆️

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 f6349a6...48484c8. Read the comment docs.

codecov · 2019-04-18T01:00:04Z

Codecov Report

Merging #23155 into dev will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev   #23155      +/-   ##
==========================================
+ Coverage   94.19%    94.2%   +<.01%     
==========================================
  Files         453      453              
  Lines       36915    36915              
==========================================
+ Hits        34773    34775       +2     
+ Misses       2142     2140       -2

Impacted Files	Coverage Δ
homeassistant/components/uk_transport/sensor.py	`93.43% <0%> (-0.73%)`	⬇️
...ssistant/components/islamic_prayer_times/sensor.py	`97.87% <0%> (+3.19%)`	⬆️

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 f6349a6...48484c8. Read the comment docs.

codecov · 2019-04-18T01:00:05Z

Codecov Report

Merging #23155 into dev will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev   #23155      +/-   ##
==========================================
+ Coverage   94.19%    94.2%   +<.01%     
==========================================
  Files         453      453              
  Lines       36915    36915              
==========================================
+ Hits        34773    34775       +2     
+ Misses       2142     2140       -2

Impacted Files	Coverage Δ
homeassistant/components/uk_transport/sensor.py	`93.43% <0%> (-0.73%)`	⬇️
...ssistant/components/islamic_prayer_times/sensor.py	`97.87% <0%> (+3.19%)`	⬆️

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 f6349a6...48484c8. Read the comment docs.

MartinHjelmare · 2019-04-18T12:39:04Z

The first PR for this integration is already in 0.92 beta, so tagging this for 0.92 too.

zxdavb · 2019-04-18T12:43:55Z

Woot! Merged!

Thanks @MartinHjelmare 💐 @GeoffAtHome 🥇

* configuration for hub tokens are now separate from host addresses/credentials * small change to docstring * use *args **kwargs

configuration for hub tokens are now separate from host addresses/cre…

d29d882

…dentials

homeassistant added cla-signed integration: geniushub small-pr PRs with less than 30 lines. labels Apr 17, 2019

OverloadUT added the in progress label Apr 17, 2019

zxdavb changed the title ~~Configuration for hub tokens is now separate from host addresses~~ Improve configuration schema for Geniushub integration Apr 17, 2019

small change to docstring

2681913

MartinHjelmare requested changes Apr 17, 2019

View reviewed changes

MartinHjelmare approved these changes Apr 17, 2019

View reviewed changes

zxdavb added 2 commits April 17, 2019 22:21

Merge remote-tracking branch 'upstream/dev' into separate_host_from_t…

6f8d211

…oken

use *args **kwargs

48484c8

MartinHjelmare merged commit 11fb486 into home-assistant:dev Apr 18, 2019

MartinHjelmare removed the in progress label Apr 18, 2019

MartinHjelmare added this to the 0.92.0 milestone Apr 18, 2019

balloob added the cherry-picked label Apr 20, 2019

balloob pushed a commit that referenced this pull request Apr 20, 2019

Improve configuration schema for Geniushub integration (#23155)

df02879

* configuration for hub tokens are now separate from host addresses/credentials * small change to docstring * use *args **kwargs

Uh oh!

Conversation

zxdavb commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Change:

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Uh oh!

MartinHjelmare Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

zxdavb Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

zxdavb Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

zxdavb Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 18, 2019

Codecov Report

Uh oh!

codecov Bot commented Apr 18, 2019

Codecov Report

Uh oh!

codecov Bot commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MartinHjelmare commented Apr 18, 2019

Uh oh!

zxdavb commented Apr 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zxdavb commented Apr 17, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

zxdavb Apr 17, 2019 •

edited

Loading

codecov Bot commented Apr 18, 2019 •

edited

Loading