Allow toggle to accept parameters by michaelarnauts · Pull Request #4745 · home-assistant/core

michaelarnauts · 2016-12-05T10:13:19Z

Description:
This PR modifies the toggle service to accept parameters. Those parameters could be used to toggle a light and turn it on full brightness if it was dimmed before it was turned off.

See #4360 for a use case.

Basicly, it allows you to do this:

script:
  toggle_light:
    sequence:
    - service: light.toggle
      entity_id: light.links
      data:
        brightness: 255

instead of this workaround:

script:
  toggle_light:
    sequence:
    - service_template: script.toggle_light_{% if is_state('light.links', 'off') %}on{% else %}off{% endif %}
  toggle_light_on:
    sequence:
    - service: light.turn_on
      entity_id: light.links
      data:
        brightness: 255
  toggle_light_off:
    sequence:
    - service: light.turn_off
      entity_id: light.links

Related issue (if applicable): fixes #4360

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:

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

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

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.

mention-bot · 2016-12-05T10:13:20Z

@michaelarnauts, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @jaharkes and @pvizeli to be potential reviewers.

pvizeli

You solution look like a bit of hack. service_toggle is a core Service and used on many component.

So you need add unittest for your changes.

pvizeli · 2016-12-05T15:25:17Z

    ATTR_FLASH: vol.In([FLASH_SHORT, FLASH_LONG]),
 })

-LIGHT_TOGGLE_SCHEMA = vol.Schema({


Don't remove this, extend your Schema.

I copied the schema from turn_on

pvizeli · 2016-12-05T15:25:46Z

    hass.services.async_register(
        DOMAIN, SERVICE_TOGGLE, async_handle_light_service,
-        descriptions.get(SERVICE_TOGGLE), schema=LIGHT_TOGGLE_SCHEMA)
+        descriptions.get(SERVICE_TOGGLE), schema=LIGHT_TURN_ON_SCHEMA)


Turn on is not toggle. see above.

The point is that SERVICE_TOGGLE can do everything SERVICE_TURNON can do, therefore the LIGHT_TOGGLE_SCHEMA is identical to LIGHT_TOGGLE_SCHEMA. Do you still want to make a seperate schema for this? Should I do something like the following then?

LIGHT_TOGGLE_SCHEMA = LIGHT_TURN_ON_SCHEMA.extend()

pvizeli · 2016-12-05T15:26:11Z

      description: Duration in seconds it takes to get to next state
      example: 60

+    rgb_color:


Don't copy stuff. Make a comment of possibility

How should I do this?

balloob · 2016-12-05T16:14:10Z

This is going to affect every single component and platform that is using ToggleEntity and thus all their function parameters have to be updated.

It's also going to be very difficult to actually implement in every component because now you need to create service parameter attribute validation for every toggle platform that validates as both TURN_ON and TURN_OFF schemas.

Having a bunch of attributes be either used for turn on or turn off seems like a bad idea to me.

michaelarnauts · 2016-12-05T16:19:48Z

@balloob should all calls be modified? The tests are running fine. Currently, they are not passing parameters, so if I'm correctly, nothing has to change if the component doesn't want to pass parameters to toggle.

Before my change, there was a parameter transition that you could pass on to toggle, but a few weeks ago (ee5f228#diff-2f6e7e067d836cc94edba0d7d51d0241L316), the function signature was changed, and the parameters were dropped from toggle making the transition parameter acutally broken.

michaelarnauts · 2016-12-05T16:27:24Z

What about passing only the parameters to turn_on then, and leaving turn_off parameterless? I don't mind about the parameters for turn_off (although transition is a good example where you might need it for turn_on and turn_off).

Do note that ToggleEntity used to behave this way before Nov 4.

balloob · 2016-12-05T16:52:49Z

In your referenced commit I made sure that the signature was exactly the same as the signature from the method that it was overriding in ToggleEntity. The turn_on and turn_off methods expect the input to be processed by voluptuous and how the service handler processed it.

I think that the solution here is to make toggle filter the keywords that are routed to either the turn_on / turn_off commands making sure they only get their expected values and correctly formatted just as they would have been passed in by the service handler. So resolve color profiles, resolve color name to rgb color etc etc.

This will make it backwards compatible with the existing toggle and will make sure that platforms only have to expect 1 set of data in their platforms.

michaelarnauts · 2016-12-05T17:06:14Z

Hmm, sounds logical, but I'm afraid that's something I can't do without some help. Shouldn't voloptuous filter out wrong parameters automatically? Should the SCHEMA for toggle be dropped then, since it will be the one from toggle on or toggle off?

balloob · 2016-12-06T02:05:59Z

Once the toggle service gets called, we're inside the entity. Data validation happens before toggle gets called so we don't know if it is to turn on or off.

balloob · 2016-12-06T04:57:39Z

We could circumvent this by having toggle actually call a service to turn_on/turn_off the entity… (which should fully happen async or else we can get in deadlocks)

pvizeli · 2016-12-06T22:09:46Z

Yeah, I think also we should call the service for handle this case. But the challenge is to handle/filter params to right service...

balloob · 2016-12-07T06:22:33Z

I think it will mean that toggle is no longer handled on an entity but instead 100% handled inside a service.

houndci-bot · 2017-01-03T20:51:45Z

+            return self.async_turn_off(**kwargs)
        else:
-            return self.async_turn_on()
+            return self.async_turn_on(**kwargs)


no newline at end of file

houndci-bot · 2017-01-03T20:57:09Z

        else:
-            return self.async_turn_on()
+            return self.async_turn_on(**kwargs)
+


blank line at end of file

houndci-bot · 2017-01-03T20:57:09Z

        else:
-            return self.async_turn_on()
+            return self.async_turn_on(**kwargs)
+


blank line at end of file

michaelarnauts · 2017-01-03T20:58:15Z

Hmm, aren't "no newline at end of file" and "blank line at end of file" two conflicting remarks?

balloob · 2017-01-03T23:01:44Z

No, there is a difference between nothing, \n and \n\n. No new line means the last line is not terminated with a new line char – some editors do this. The blank line at end means there are two or more new line characters at the end.

balloob · 2017-01-24T17:02:30Z

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

Allow toggle to accept parameters

66d5988

pvizeli reviewed Dec 5, 2016

View reviewed changes

Use LIGHT_TOGGLE_SCHEMA for SERVICE_TOGGLE

f83d041

balloob self-assigned this Dec 6, 2016

Merge branch 'dev' into light_toggle_attr

134838a

houndci-bot reviewed Jan 3, 2017

View reviewed changes

newline. dropped @asyncio.coroutine

92d9672

houndci-bot reviewed Jan 3, 2017

View reviewed changes

balloob closed this Jan 24, 2017

home-assistant locked and limited conversation to collaborators Apr 30, 2017

Uh oh!

Conversation

michaelarnauts commented Dec 5, 2016

Uh oh!

mention-bot commented Dec 5, 2016

Uh oh!

pvizeli 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!

pvizeli Dec 5, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

balloob commented Dec 5, 2016

Uh oh!

michaelarnauts commented Dec 5, 2016

Uh oh!

michaelarnauts commented Dec 5, 2016

Uh oh!

balloob commented Dec 5, 2016

Uh oh!

michaelarnauts commented Dec 5, 2016

Uh oh!

balloob commented Dec 6, 2016

Uh oh!

balloob commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvizeli commented Dec 6, 2016

Uh oh!

balloob commented Dec 7, 2016

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!

michaelarnauts commented Jan 3, 2017

Uh oh!

balloob commented Jan 3, 2017

Uh oh!

balloob commented Jan 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pvizeli Dec 5, 2016 •

edited

Loading

balloob commented Dec 6, 2016 •

edited

Loading