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

Adding color support to template light #12439

Closed
wants to merge 7 commits into from
Closed

Conversation

grea09
Copy link
Contributor

@grea09 grea09 commented Feb 15, 2018

I wanted to add color to the template light for a project and I thought I could submit it. I don't know much of the rest of the code of the project but the changes work so far on my installation.

Description:

Related issue (if applicable): fixes https://community.home-assistant.io/t/light-template-colors/33937/2

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New 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:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

I wanted to add color to the template light for a project and I thought I could submit it. I don't know much of the rest of the code of the project but the changes work so far on my installation.
@homeassistant homeassistant added merging-to-master This PR is merging into the RC branch and should probably change the branch to `dev`. platform: light.template cla-needed labels Feb 15, 2018
@homeassistant
Copy link
Contributor

Hi @grea09,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

self._state = None

self._color = color
#TODO input check for colors

Choose a reason for hiding this comment

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

block comment should start with '# '

@@ -156,9 +184,12 @@ def name(self):
@property
def supported_features(self):
"""Flag supported features."""
if self._level_script is not None:
if self._level_script is not None and self._color_script is not None:
return SUPPORT_RGB_COLOR |SUPPORT_BRIGHTNESS

Choose a reason for hiding this comment

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

missing whitespace around operator


def __init__(self, hass, device_id, friendly_name, state_template,
icon_template, entity_picture_template, on_action,
off_action, level_action, level_template, entity_ids):
off_action, level_action, level_template, color_action, color_template, entity_ids):

Choose a reason for hiding this comment

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

line too long (101 > 79 characters)

@@ -96,7 +112,7 @@ def async_setup_platform(hass, config, async_add_devices, discovery_info=None):
LightTemplate(
hass, device, friendly_name, state_template,
icon_template, entity_picture_template, on_action,
off_action, level_action, level_template, entity_ids)
off_action, level_action, level_template, color_action, color_template, entity_ids)

Choose a reason for hiding this comment

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

line too long (99 > 79 characters)

@@ -11,7 +11,7 @@

from homeassistant.core import callback
from homeassistant.components.light import (
ATTR_BRIGHTNESS, ENTITY_ID_FORMAT, Light, SUPPORT_BRIGHTNESS)
ATTR_BRIGHTNESS, ATTR_RGB_COLOR, ENTITY_ID_FORMAT, Light, SUPPORT_BRIGHTNESS, SUPPORT_RGB_COLOR)

Choose a reason for hiding this comment

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

line too long (100 > 79 characters)

@balloob balloob changed the base branch from master to dev February 15, 2018 21:38
@balloob balloob removed the merging-to-master This PR is merging into the RC branch and should probably change the branch to `dev`. label Feb 15, 2018
grea09 added a commit to grea09/home-assistant.github.io that referenced this pull request Mar 1, 2018
These modifications go along [this pull request](home-assistant/core#12439).
I added an example for a common classes of IR remote lights.

def __init__(self, hass, device_id, friendly_name, state_template,
icon_template, entity_picture_template, on_action,
off_action, level_action, level_template, entity_ids):
off_action, level_action, level_template,

Choose a reason for hiding this comment

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

trailing whitespace

@@ -96,7 +113,8 @@ def async_setup_platform(hass, config, async_add_devices, discovery_info=None):
LightTemplate(
hass, device, friendly_name, state_template,
icon_template, entity_picture_template, on_action,
off_action, level_action, level_template, entity_ids)
off_action, level_action, level_template,

Choose a reason for hiding this comment

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

trailing whitespace

@@ -11,7 +11,8 @@

from homeassistant.core import callback
from homeassistant.components.light import (
ATTR_BRIGHTNESS, ENTITY_ID_FORMAT, Light, SUPPORT_BRIGHTNESS)
ATTR_BRIGHTNESS, ATTR_RGB_COLOR, ENTITY_ID_FORMAT, Light,

Choose a reason for hiding this comment

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

trailing whitespace

@grea09
Copy link
Contributor Author

grea09 commented Mar 1, 2018

I encounter errors once I updated the code to the latest version and I have no idea of why it behaves like this. Here is the error in my configuration :
ERROR:homeassistant.config:Invalid config for [light.template]: template value is None for dictionary value @ data['lights']['led_strip']['color_template']. Got None

I tried to tweak the schema but nothing seems to fit. Can someone with more understanding of the global picture point me in the right direction ? Should I take this issue to the forums ?

@grea09
Copy link
Contributor Author

grea09 commented Mar 12, 2018

I figured out the errors (bad merge of changes I guess). It works in my installation like a charm and I would like for it to be merged in the main code in order to be able to update without worries.

Thanks for the great software !

frenck pushed a commit to grea09/home-assistant.github.io that referenced this pull request Mar 23, 2018
These modifications go along [this pull request](home-assistant/core#12439).
I added an example for a common classes of IR remote lights.
@grea09
Copy link
Contributor Author

grea09 commented Apr 4, 2018

Is there any additional step required to merge this ? I would like to be able to update my installation.
Thanks in advance.

Using SUPPORT_COLOR instead of the deprecated SUPPORT_RGB_COLOR
@grea09
Copy link
Contributor Author

grea09 commented Apr 4, 2018

I would appreciate a lot that this code gets merged because maintaining it outside of the main code is become a regular hassle. Thanks in advance.

@grea09
Copy link
Contributor Author

grea09 commented Apr 12, 2018

Apparently RGB colors are not a thing anymore in the light environment ? Help would be greatly appreciated.

@dgomes
Copy link
Contributor

dgomes commented May 5, 2018

Code LGTM, but:

  • Please fill in the PR template
  • Consider adding a test

@dgomes
Copy link
Contributor

dgomes commented May 6, 2018

Forgot about something:

We have since standardised around Hue, have a look at other light platforms like MQTT:

https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/light/mqtt.py

@ReX1983
Copy link

ReX1983 commented Jun 6, 2018

Is there any update on this? @grea09, I've tried to use this version as custom component but for some reason the component stopped working after merging your changes.

@grea09
Copy link
Contributor Author

grea09 commented Jun 11, 2018

I will try to correct this code once I get more available time. The code needs to take the recent changes to work and is defunct for now with the latest version of HA.

@fabaff
Copy link
Member

fabaff commented Aug 7, 2018

This PR seems to have gone stale. Please re-open it when you want to proceed.

@fabaff fabaff closed this Aug 7, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants