Issue/add template fans by giangvo · Pull Request #12027 · home-assistant/core

giangvo · 2018-01-29T11:03:24Z

Description:

Add template fan

Example entry for `configuration.yaml`:

fan:
  - platform: template
    fans:
      bedroom_fan:
        friendly_name: "Bedroom fan" #Optional
        value_template: "{{ states('input_boolean.state') }}" #Required
        speed_template: "{{ states('input_select.speed') }}" #Optional
        oscillating_template: "{{ states('input_select.osc') }}" #Optional
        turn_on: #Required
          service: script.fan_on
        turn_off: #Required
          service: script.fan_off
        set_speed: #Optional
          service: script.fan_speed
          data_template:
            speed: "{{ speed }}"
        set_oscillating: #Optional
          service: script.fan_oscillating
          data_template:
            oscillating: "{{ oscillating }}"
        speeds: #Optional
          - '1'
          - '2'
          - '3'

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.

This reverts commit 1e05371.

…ssue/add-template-fans

todschmidt · 2018-01-29T19:40:03Z

Nice work. Can you add documentation for it as well?

giangvo · 2018-01-30T00:58:06Z

Create PR for adding document here: home-assistant/home-assistant.io#4551

giangvo · 2018-02-01T21:46:14Z

Could someone review this?
Thanks

thomasdelaet · 2018-02-02T20:02:05Z

I am using the previous version of this component and it works perfectly, so I assume this one does too.

Thanks a lot for resubmitting this and I hope it gets merged soon.

todschmidt · 2018-02-03T16:22:14Z

Just a couple small changes and I'll merge this, thanks for adding it.

todschmidt · 2018-02-03T16:15:55Z

+CONF_OSCILLATING_TEMPLATE = 'oscillating_template'
+CONF_ON_ACTION = 'turn_on'
+CONF_OFF_ACTION = 'turn_off'
+CONF_SET_SPEED_ACTION = 'set_speed'


I don't think you should add _ACTION here, just CONF_SET_SPEED, etc.

I am using same naming convention as in light/template so prefer to keep them if u don't might

todschmidt · 2018-02-03T16:17:58Z

+CONF_SPEED_LIST = 'speeds'
+CONF_SPEED_TEMPLATE = 'speed_template'
+CONF_OSCILLATING_TEMPLATE = 'oscillating_template'
+CONF_ON_ACTION = 'turn_on'


You should use the consts COMMAND_ON and COMMAND_OFF to match the other templates.

light/template is also using same naming convention and some people might already use my PR so I prefer not to change this.

todschmidt · 2018-02-03T16:20:59Z

+
+from homeassistant.core import callback
+from homeassistant import setup
+import homeassistant.components as core


Don't use core here, use component

todschmidt · 2018-02-09T16:22:36Z

Sorry, I hadn't committed my comments so they weren't showing up for you. I think this is fine to merge if you can just clean up those couple things. Nice job adding all the tests up front.

todschmidt · 2018-02-10T22:44:18Z

@balloob any objections to merging this?

colindunn · 2018-02-27T03:47:00Z

Was using this as a custom component until merged, but it no longer works with the voluptuous changes for 0.64 just to let you know

balloob · 2018-02-28T04:32:48Z

+_VALID_STATES = [STATE_ON, STATE_OFF, 'true', 'false']
+
+FAN_SCHEMA = vol.Schema({
+    vol.Optional(CONF_FRIENDLY_NAME, default=None): cv.string,


Remove default=None from your schema. It's not allowed to have a default that is not a valid value.

balloob · 2018-02-28T04:33:33Z

+
+
+@asyncio.coroutine
+def async_setup_platform(hass, config, async_add_devices, discovery_info=None):


We no longer use @asyncio.coroutine, easy to fix by removing the decorator and instead add async in front of func name: async def async_setup_platform(…. All yield from will have to be replaced with await

balloob · 2018-02-28T04:34:45Z

+        template_entity_ids = set()
+
+        temp_ids = state_template.extract_entities()
+        if str(temp_ids) != MATCH_ALL:


Why stringify? you can compare a list with a string.

Also, if it is match_all, shouldn't you make sure that you match all entity ids? Now if 1 is match all but other 2 are not, you will not match all.

Honestly, I copied it from light/template so I don't understand it 100%. It is working as light template so I think its good enough to be merged.

Please remove the call to str() and compare temp_ids directly to MATCH_ALL.

balloob · 2018-02-28T04:35:01Z

+        )
+
+    async_add_devices(fans)
+    return True


Remove this. Platforms don't return a value.

…ssue/add-template-fans

giangvo · 2018-02-28T05:46:40Z

@balloob , I updated the code as your comments (except the "stringify" one). Please review again.
Thanks!

balloob · 2018-02-28T19:49:14Z

+
+        if speed_template:
+            temp_ids = speed_template.extract_entities()
+            if str(temp_ids) != MATCH_ALL:


remove str

balloob · 2018-02-28T19:50:41Z

+                template_entity_ids |= set(temp_ids)
+
+        if oscillating_template:
+            temp_ids = oscillating_template.extract_entities()


if one is MATCH_ALL, they should all be MATCH_ALL. This is not currently the case. If the oscillating_template returns MATCH_ALL, it will just use extracted entities of the other templates.

balloob · 2018-03-01T06:55:16Z

+
+        if speed_template:
+            temp_ids = speed_template.extract_entities()
+            if temp_ids != MATCH_ALL:


This still won't work. The moment you get a MATCH_ALL, you should just match on MATCH_ALL.

Sorry but I dont get it, could u give the correct code.
Thanks

You could take a look at how I recently did it in the template sensor platform:

https://github.com/home-assistant/home-assistant/blob/8ed302402603c60ff71d5c0baebec269b3769b46/homeassistant/components/sensor/template.py#L59-L72

OttoWinter · 2018-03-18T12:02:47Z

+
+        if speed_template:
+            temp_ids = speed_template.extract_entities()
+            if temp_ids != MATCH_ALL:


You could take a look at how I recently did it in the template sensor platform:

https://github.com/home-assistant/home-assistant/blob/8ed302402603c60ff71d5c0baebec269b3769b46/homeassistant/components/sensor/template.py#L59-L72

OttoWinter · 2018-03-18T12:05:51Z

+
+        This method is a coroutine.
+        """
+        self._state = False


Shouldn't we only set the state to False once the off script terminates? + If the off script raises an exception I would expect the state to remain in the previous state.

OttoWinter · 2018-03-18T12:08:36Z

+            self.async_schedule_update_ha_state(True)
+
+        self.hass.bus.async_listen_once(
+            EVENT_HOMEASSISTANT_START, template_fan_startup)


I don't understand why we should only call this on EVENT_HOMEASSISTANT_START. This code would break once we start dynamically loading/unloading entities. Maybe have a look at how it's done in the light group platform:

https://github.com/home-assistant/home-assistant/blob/ca5f4709564773c8cebd6ee40aa7cc1095b19105/homeassistant/components/light/group.py#L72-L82

I am doing the same as in light/template and sensor/template. Using the code in light group did not work. Fan is no changed when other entities changed

OttoWinter · 2018-03-18T12:09:11Z

+
+    async def async_update(self):
+        """Update the state from the template."""
+        _LOGGER.info('Updating fan %s', self._name)


This seems like quite the unnecessary logging statement. Even more it's info and not debug.

OttoWinter · 2018-03-18T12:10:56Z

+            self._state = None
+
+        # Validate state
+        if state in _VALID_STATES:


I think if the previous call produces a TemplateError, state will not be set, resulting in NameError: name 'state' is not defined.

OttoWinter · 2018-03-18T12:15:46Z

+            self._state = None
+
+        # Update speed if 'speed_template' is configured
+        if self._speed_template:


PEP8 recommend using is not None:

if self._speed_template is not None:

OttoWinter · 2018-03-18T12:17:44Z

+                {ATTR_OSCILLATING: oscillating}
+            )
+        else:
+            _LOGGER.error(


This check is already handled by the service schema:

https://github.com/home-assistant/home-assistant/blob/678f284015a2c52f96a7687979cfd9f785e4527a/homeassistant/components/fan/__init__.py#L78-L81

Also if it were needed, it should definitely not go into this platform, but rather in the core fan definition.

OttoWinter · 2018-03-18T12:18:40Z

+    async def async_turn_on(self, speed: str = None) -> None:
+        """Turn on the fan.
+
+        This method is a coroutine.


I think by now (especially with async def) we don't need to plaster "This method is a coroutine." everywhere where it's abundantly clear from the context.

OttoWinter · 2018-03-18T12:21:26Z

+                }
+            })
+
+        self.hass.start()


No need to call self.hass.start() in any of these test cases.

removing them failed the tests.

OttoWinter · 2018-03-18T12:23:40Z

+
+        with assert_setup_component(2, 'input_select'):
+            assert setup.setup_component(self.hass, 'input_select', {
+                'input_select': {


I mean lots of tests do this, but I don't think tests from one platform should depend on another integration, it's just bad testing (and makes changing those other integrations a pain).

Why not just use the state machine manually?

I want them to be integration tests so we can sure that our scripts run successfully with real results. Please advise an example of state machine way of testing.

thomasdelaet · 2018-04-07T22:04:56Z

Would be really great to have this one merged! I'm happy to create a new pull request covering the requested changes.

…ssue/add-template-fans

giangvo · 2018-04-30T23:01:00Z

@OttoWinter could you please review. I updated the code as your comments.

'on'/'off' are only accepted values for value_template
'True/Falseare only accepted values forosc_template` (because it is defined in the schema)

FAN_OSCILLATE_SCHEMA = vol.Schema({
    vol.Required(ATTR_ENTITY_ID): cv.entity_ids,
    vol.Required(ATTR_OSCILLATING): cv.boolean
})

Concerns addressed

balloob · 2018-05-02T21:45:27Z

Looks good 🐬

thomasdelaet · 2018-05-03T08:40:30Z

Fantastic!

giangvo added 10 commits November 1, 2017 08:48

add template fan

440eed3

add-template: address PR comments

42944b2

add-template: remove unused import

514aaf6

add-template: revert async_track_state_change change

494b7a4

add-template: use yield from

1e05371

Revert "add-template: use yield from"

4043a14

This reverts commit 1e05371.

add-template: use yield

e9da52f

add-template: remove unused import

38e2540

Merge branch 'dev' of github.com:home-assistant/home-assistant into i…

5bfd46a

…ssue/add-template-fans

add-template: remove async_add_job usages

5cd86aa

homeassistant added cla-signed new-platform platform: fan.template labels Jan 29, 2018

giangvo mentioned this pull request Jan 29, 2018

add template fan #10261

Closed

3 tasks

giangvo mentioned this pull request Jan 30, 2018

add-template: add document for template fan home-assistant/home-assistant.io#4551

Closed

2 tasks

todschmidt reviewed Feb 9, 2018

View reviewed changes

use components

b4d97ab

homeassistant added the cla-signed label Feb 10, 2018

balloob reviewed Feb 28, 2018

View reviewed changes

giangvo added 3 commits February 28, 2018 15:47

add-template: use async/await

a1b08d6

Merge branch 'dev' of github.com:home-assistant/home-assistant into i…

2ff6183

…ssue/add-template-fans

add-template: fix style

a5900b2

homeassistant added the cla-signed label Feb 28, 2018

balloob reviewed Feb 28, 2018

View reviewed changes

add-template: remove str()

ed10ec8

homeassistant added the cla-signed label Feb 28, 2018

balloob reviewed Mar 1, 2018

View reviewed changes

OttoWinter previously requested changes Mar 18, 2018

View reviewed changes

giangvo added 3 commits April 30, 2018 17:34

Merge branch 'dev' of github.com:home-assistant/home-assistant into i…

7afca9e

…ssue/add-template-fans

address pr comments

da2e18a

fix style

f8437df

balloob approved these changes May 2, 2018

View reviewed changes

balloob merged commit ef4498e into home-assistant:dev May 2, 2018

giangvo mentioned this pull request May 2, 2018

add document for fan template home-assistant/home-assistant.io#5296

Merged

2 tasks

balloob mentioned this pull request May 11, 2018

0.69 #14392

Merged

home-assistant locked and limited conversation to collaborators Sep 5, 2018

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



		@asyncio.coroutine
		def async_setup_platform(hass, config, async_add_devices, discovery_info=None):

Uh oh!

Conversation

giangvo commented Jan 29, 2018

Description:

Example entry for configuration.yaml:

Checklist:

Uh oh!

todschmidt commented Jan 29, 2018

Uh oh!

giangvo commented Jan 30, 2018

Uh oh!

giangvo commented Feb 1, 2018

Uh oh!

thomasdelaet commented Feb 2, 2018

Uh oh!

todschmidt commented Feb 3, 2018

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!

todschmidt commented Feb 9, 2018

Uh oh!

todschmidt commented Feb 10, 2018

Uh oh!

colindunn commented Feb 27, 2018

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!

giangvo commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Example entry for `configuration.yaml`:

giangvo commented Feb 28, 2018 •

edited

Loading