Template light by cribbstechnologies · Pull Request #7657 · home-assistant/core

cribbstechnologies · 2017-05-19T01:02:07Z

Description:

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

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

light:
  - platform: template
    lights:
      temp_switch:
        value_template: {{1 == 1}}
        turn_on: 
          service: light.turn_on
          entity_id: light.other_light
        turn_off:
          service: light.turn_off
          entity_id: light.other_light
        set_level:
           service: light.turn_on
           data_template: {{brightness}}
        level_template: {{42}}

Checklist:

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

Documentation added/updated in home-assistant.github.io

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.

cribbstechnologies · 2017-05-19T01:03:09Z

I could not figure out how to undo the auto-add and subsequent commit to the polymer submodule, I tried to straighten it out and I may have actually made it worse. I would've just cut a new branch and cherry picked but it was part of a commit that contained useful information

This reverts commit 852c87f.

cribbstechnologies · 2017-05-21T01:52:26Z

I fixed it! This should be good to go now. @Landrash requested I write a blog post to accompany this but I haven't had a chance to do that yet.

emlove

Thanks for the PR. This will be a nice addition! Looks good at the core, just some small stuff here.

emlove · 2017-05-22T15:10:29Z

+_VALID_STATES = [STATE_ON, STATE_OFF, 'true', 'false']
+
+CONF_LIGHTS = 'lights'
+ON_ACTION = 'turn_on'


Prefix these with CONF_

emlove · 2017-05-22T15:14:00Z

+    vol.Optional(CONF_VALUE_TEMPLATE, default=None): cv.template,
+    vol.Optional(LEVEL_ACTION, default=None): cv.SCRIPT_SCHEMA,
+    vol.Optional(LEVEL_TEMPLATE, default=None): cv.template,
+    vol.Optional(ATTR_FRIENDLY_NAME, default=None): cv.string,


Import CONF_FRIENDLY_NAME for this instead.

emlove · 2017-05-22T15:25:46Z

+    vol.Optional(LEVEL_ACTION, default=None): cv.SCRIPT_SCHEMA,
+    vol.Optional(LEVEL_TEMPLATE, default=None): cv.template,
+    vol.Optional(ATTR_FRIENDLY_NAME, default=None): cv.string,
+    vol.Optional(ATTR_ENTITY_ID): cv.entity_ids


Template switch actually also has this wrong, but this should be CONF_ENTITY_ID. They end up the same functionally, but these should technically only be used within their proper domain to be correct.

emlove · 2017-05-22T15:28:13Z

+        off_action = device_config[OFF_ACTION]
+        level_action = device_config[LEVEL_ACTION]
+        level_template = device_config[LEVEL_TEMPLATE]
+        entity_ids = []


If no templates are defined, this list still needs to include the configured entity IDs.

emlove · 2017-05-22T15:29:16Z

+
+        if state_template is not None:
+            entity_ids = (device_config.get(ATTR_ENTITY_ID) or
+                          state_template.extract_entities())


If no entity ID list is defined, entity_ids needs to be a union of all both template extract_entities() results.

Given this and the comment above, I'd generate the list of entity_ids from the templates, and then set

entity_ids = device_config.get(CONF_ENTITY_ID, template_entity_ids)

emlove · 2017-05-22T15:32:07Z

+        if state_template is not None:
+            entity_ids = (device_config.get(ATTR_ENTITY_ID) or
+                          state_template.extract_entities())
+            state_template.hass = hass


We're actually setting this in the constructor below, so we can drop it here.

emlove · 2017-05-22T15:33:18Z

+        self._state = False
+
+    @asyncio.coroutine
+    def async_update(self):


Include should_poll False, as done here.

emlove · 2017-05-22T15:35:43Z

+                    self._state = None
+
+            if self._level_template is not None:
+                brightness = self._level_template.async_render()


We need error checking here similar to above. If it doesn't match an integer 0-255, we should log a similar error and set self._brightness = None.

emlove · 2017-05-22T15:36:28Z

+
+        except TemplateError as ex:
+            _LOGGER.error(ex)
+            self._state = None


Since there are two renders here, wrap each one separately, and set the appropriate state to None. It's safe to have this try only wrap the lines with async_render().

emlove · 2017-05-22T15:41:43Z

+    @asyncio.coroutine
+    def async_turn_on(self, **kwargs):
+        """Turn the light on."""
+        self._state = True


Only optimistically set the status if there is no template defined. Same for brightness and turn_off below. If the status is optimistically updated, run self.hass.async_add_job(self.async_update_ha_state()) to update the state. Note that we don't need to pass True, since async_update isn't necessary.

houndci-bot · 2017-05-22T17:44:25Z

+                brightness = self._level_template.async_render()
+            except TemplateError as ex:
+                _LOGGER.error(ex)
+                self._state = None    


trailing whitespace

houndci-bot · 2017-05-22T17:44:25Z

+            except TemplateError as ex:
+                _LOGGER.error(ex)
+                self._state = None
+


blank line contains whitespace

emlove

Next Round.

It would also be good to add some tests for the optimistic state logic, as it almost slipped through only halfway done.

emlove · 2017-05-22T21:52:06Z

+        level_template = device_config[CONF_LEVEL_TEMPLATE]
+
+        template_entity_ids = []
+        entity_ids = []


entity_ids = [] can be removed.

emlove · 2017-05-22T21:52:48Z

+
+        if state_template is not None:
+            temp_ids = state_template.extract_entities()
+            if str(temp_ids) != '*':


Instead, check if temp_ids != MATCH_ALL:, where MATCH_ALL is imported from homeassistant.const.

emlove · 2017-05-22T21:58:44Z

+        if state_template is not None:
+            temp_ids = state_template.extract_entities()
+            if str(temp_ids) != '*':
+                template_entity_ids = list(set(template_entity_ids) |


Looking at this, now, we can actually make this even simpler. Let's initialize template_entity_ids, as an empty set, then here we can just write template_entity_ids |= set(temp_ids). We can actually pass the set right to async_track_state_change without casting it back. Also though, since we're now filtering out MATCH_ALL, we need to catch the scenario where none of the templates have matches, so at the end you'll need to check

if not template_entity_ids: template_entity_ids = MATCH_ALL

emlove · 2017-05-22T21:59:42Z

+        self._state = False
+        self._brightness = None
+        self._entities = entity_ids
+        self._entities.append(self.entity_id)


Why was this added? We don't want to recursively update when this entity is changed.

emlove · 2017-05-22T22:11:59Z

+            # Determine need to evaluate template before returning.
+            yield_for_render = (self._template is not None or
+                                self._level_template is not None)
+            self.hass.async_add_job(


I think I understand the confusion now. This can always be async_update_ha_state(True), since if we don't change the internal attributes here, updating the ha state will never do anything.

But, you're right that we shouldn't ignore it either. What we should do is if self._template and self._level_template are both None, we shouldn't even schedule these updates here. There's no reason to listen for any state changes if there are no templates that need to be re-rendered.

emlove · 2017-05-22T22:19:50Z

+            # Determine need to evaluate template before returning.
+            yield_for_render = (self._template is not None or
+                                self._level_template is not None)
+            self.hass.async_add_job(


I think here we should just run it with async_update_ha_state(True). This one is only run once, so it's not a huge deal. The big concern was the updates in the service methods. Sorry for the miscommunication.

emlove · 2017-05-23T00:00:55Z

+            """Handle target device state changes."""
+            if (
+                self._template is not None or
+                    self._level_template is not None):


This condition should be above line 163

emlove · 2017-05-23T00:01:49Z

+        self.hass.async_add_job(self._off_script.async_run())
+        if self._template is None:
+            self._state = False
+        self.hass.async_add_job(self.async_update_ha_state())


Indent this so it only runs if there is no template.

emlove

LGTM!

emlove · 2017-05-24T18:32:50Z

Thanks for your contribution! 🎉

cribbstechnologies added 8 commits May 17, 2017 12:13

starting light template component

9116b5a

linting/flaking

01b675a

starting unit tests from copypasta

7693083

working on unit testing

c2fa266

forgot to commit the test

876f270

wrapped up unit testing

43645d2

adding remote back

852c87f

updates post running tox

a4cfcdf

homeassistant added new-platform platform: light.template cla-signed labels May 19, 2017

balloobbot added the platform label May 19, 2017

cribbstechnologies mentioned this pull request May 19, 2017

Template Light Platform documentation home-assistant/home-assistant.io#2661

Merged

Revert "adding remote back"

2e5af74

This reverts commit 852c87f.

homeassistant added the cla-signed label May 20, 2017

adding submodule back from origin

c47a10a

updating submodule

287ed13

homeassistant added the cla-signed label May 22, 2017

cribbstechnologies added 5 commits May 22, 2017 10:37

removing a line to commit

e6cb570

re-adding line

1da1dff

trying to update line endings

044e2a5

trying to fix line endings

4f26997

trying a different approach

9136f38

emlove requested changes May 22, 2017

View reviewed changes

making requested changes, need to fix tests

9aa2c53

houndci-bot reviewed May 22, 2017

View reviewed changes

cribbstechnologies added 2 commits May 22, 2017 13:45

flaking

f902649

union rather than intersect; makes a big difference

2b0d4b2

cribbstechnologies added 2 commits May 22, 2017 15:22

more tests passing, not sure why this one's failing

df577a7

got it working

67d0655

emlove requested changes May 22, 2017

View reviewed changes

most of the requested changes

62ffa4b

emlove reviewed May 23, 2017

View reviewed changes

cribbstechnologies added 2 commits May 22, 2017 20:11

hopefully done now

d63b87a

sets; the more you know

b004f8d

emlove approved these changes May 23, 2017

View reviewed changes

emlove merged commit ef4ef2d into home-assistant:dev May 24, 2017

cribbstechnologies deleted the template-light branch May 30, 2017 15:54

balloob mentioned this pull request Jun 2, 2017

0.46 #7866

Merged

home-assistant locked and limited conversation to collaborators Sep 4, 2017

ghost added integration: template and removed platform: light.template labels Mar 21, 2019

Uh oh!

Conversation

cribbstechnologies commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

cribbstechnologies commented May 19, 2017

Uh oh!

cribbstechnologies commented May 21, 2017

Uh oh!

emlove left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emlove left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emlove left a comment

Choose a reason for hiding this comment

Uh oh!

emlove commented May 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cribbstechnologies commented May 19, 2017 •

edited

Loading

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