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.

Sign up for GitHub

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

Jump to bottom

Add config entry for LIFX #17201

Merged
merged 5 commits into from
Oct 7, 2018
Merged

Add config entry for LIFX #17201

merged 5 commits into from
Oct 7, 2018

Conversation

amelchio
Copy link
Contributor

@amelchio amelchio commented Oct 6, 2018 •
edited
Loading

Description:

This is a minimal PR to add a config entry for LIFX. I plan additional PRs for a more complete experience.

Breaking change: LIFX can now be configured from the integrations page in the config panel and advanced configuration is available through the lifx: component configuration. Configuring LIFX via light platform config no longer works.

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

Example entry for configuration.yaml (if applicable):

lifx:
  light:
    server: 10.0.2.14
    broadcast: 10.0.2.255

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.

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.

@amelchio amelchio requested a review from a team as a code owner October 6, 2018 20:45
@ghost ghost assigned amelchio Oct 6, 2018
@ghost ghost added the in progress label Oct 6, 2018
@amelchio amelchio mentioned this pull request Oct 6, 2018
Merged
2 tasks
MartinHjelmare
MartinHjelmare reviewed Oct 7, 2018
View reviewed changes
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.

There's no config schema for the component. Better add that and let the core do the config validation instead of doing that later in the platform.

homeassistant/components/light/lifx.py Outdated
@@ -30,9 +32,10 @@

_LOGGER = logging.getLogger(__name__)

REQUIREMENTS = ['aiolifx==0.6.3', 'aiolifx_effects==0.2.1']
DEPENDENCIES = ('lifx',)
Copy link
Member

Choose a reason for hiding this comment

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

We normally use a list here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I thought that looked odd. (It was copied from the Cast migration but I see now that this is not the normal way.)

@amelchio
Copy link
Contributor Author

amelchio commented Oct 7, 2018

I refrained from moving more stuff into the component in order to keep the PR small. If you think it's better to move the config schema in this PR, I can work on that.

@balloob
Copy link
Member

balloob commented Oct 7, 2018

Since config validation is part of the component, it should be there.

balloob
balloob reviewed Oct 7, 2018
View reviewed changes
homeassistant/components/light/lifx.py Outdated
Deprecated.
"""
_LOGGER.warning('Loading LIFX via light platform config is deprecated.')
await _async_setup_platform(
Copy link
Member

Choose a reason for hiding this comment

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

Since lifx is a dependency, configuring via a platform will set up the lifx component and load the config entry. If we also call this, we end up setting it up twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the backwards compatibility now.

balloob
balloob reviewed Oct 7, 2018
View reviewed changes
homeassistant/components/light/lifx.py Outdated
async def async_setup_entry(hass, config_entry, async_add_entities):
"""Set up LIFX from a config entry."""
try:
config = vol.Schema(CONFIG)(hass.data[LIFX_DOMAIN].get('light', {}))
Copy link
Member

Choose a reason for hiding this comment

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

Validation in component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I had to remove the defaults but maybe just as well to keep those in the third party library.

MartinHjelmare
MartinHjelmare reviewed Oct 7, 2018
View reviewed changes
homeassistant/components/light/lifx.py Outdated
@@ -17,12 +18,12 @@
from homeassistant.components.light import (
ATTR_BRIGHTNESS, ATTR_BRIGHTNESS_PCT, ATTR_COLOR_NAME, ATTR_COLOR_TEMP,
ATTR_EFFECT, ATTR_HS_COLOR, ATTR_KELVIN, ATTR_RGB_COLOR, ATTR_TRANSITION,
ATTR_XY_COLOR, COLOR_GROUP, DOMAIN, LIGHT_TURN_ON_SCHEMA, PLATFORM_SCHEMA,
ATTR_XY_COLOR, COLOR_GROUP, DOMAIN as LIGHT_DOMAIN, LIGHT_TURN_ON_SCHEMA,
Copy link
Member

Choose a reason for hiding this comment

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

Keep DOMAIN as DOMAIN, since this is the domain (light) of this platform.

homeassistant/components/light/lifx.py Outdated
local_addr = config.get(CONF_SERVER)
if local_addr is not None:
kwargs['local_addr'] = (local_addr, 0)
coro = hass.loop.create_datagram_endpoint(lambda: lifx_discovery, **kwargs)

hass.async_add_job(coro)
Copy link
Member

Choose a reason for hiding this comment

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

Use async_create_task.

MartinHjelmare
MartinHjelmare approved these changes Oct 7, 2018
View reviewed changes
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.

Looks good!

@MartinHjelmare MartinHjelmare merged commit 086c715 into home-assistant:dev Oct 7, 2018
@ghost ghost removed the in progress label Oct 7, 2018
@balloob balloob mentioned this pull request Oct 26, 2018
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
@ghost ghost removed the platform: light.lifx label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@balloob balloob balloob left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees

@amelchio amelchio

Labels
breaking-change cla-signed core Hacktoberfest integration: lifx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amelchio @balloob @MartinHjelmare @homeassistant