Allow wait template to run the remainder of the script by lhovo · Pull Request #15836 · home-assistant/core

lhovo · 2018-08-05T02:45:02Z

New feature: allow wait template to run the remainder of the script

Breaking changes
The wait template will now continue to run the remainder of the script on timeout, the original functionality can be gained by setting continue_on_timeout: false.

Description:

This allows the Wait Template script to use a timer to continue running the script if timeout is reached vs the current behaviour of exiting.
This is achieved by setting a new flag 'proceed' to 'true' in the config.
By default this flag is set to 'false' and is optional to ensure this is not a breaking change.
For this flag to work the optional 'timeout' is required to be set also.

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

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

# if the front door is open and it's night time turn on the light
# when the occupant reaches the first sensor, turn the light off
# if the timeout triggers, turn the light off anyway
automation:
  - id: '15'
    alias: Door Open Light On
    trigger:
    - entity_id: binary_sensor.front
      platform: state
      to: 'on'
    condition:
    - condition: state
      entity_id: light.entrance
      state: 'off'
    - after: '17:00'
      before: '6:00'
      condition: time
    action:
    - data:
        entity_id:
        - light.entrance
      service: homeassistant.turn_on
    - wait_template: '{{ is_state("binary_sensor.entrance", "on") }}'
      timeout: 00:01:00
      continue_on_timeout: 'true'
    - alias: ''
      data:
        entity_id:
        - light.entrance
      service: homeassistant.turn_off

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

…e script on timeout

homeassistant · 2018-08-05T02:45:02Z

Hi @lhovo,

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!

houndci-bot · 2018-08-05T02:45:08Z

homeassistant/helpers/script.py


                @callback
-                def async_script_wait(entity_id, from_s, to_s):
+                def async_script_wait(entity_id, from_s = None, to_s = None):


unexpected spaces around keyword / parameter equals

houndci-bot · 2018-08-05T02:45:08Z

homeassistant/helpers/config_validation.py


 from homeassistant.const import (
-    CONF_PLATFORM, CONF_SCAN_INTERVAL, TEMP_CELSIUS, TEMP_FAHRENHEIT,
+    CONF_PLATFORM, CONF_PROCEED, CONF_SCAN_INTERVAL, TEMP_CELSIUS, TEMP_FAHRENHEIT,


line too long (83 > 79 characters)

homeassistant · 2018-08-05T02:48:34Z

Hi @lhovo,

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!

awarecan · 2018-08-05T04:04:34Z

Thanks for your contribution. There are several outstanding issues in your pull request you have to resolve before review.

Please do not change file mod 100644 → 100755
Please fill in PR template's "Example entry for configuration.yaml" section, in your case, maybe template and script
Please add unit test, home-assistant/tests/helpers/test_script.py
Please fix the link to your document PR (remove the space between # and issue number)

lhovo · 2018-08-05T10:56:02Z

Please do not change file mod 100644 → 100755
Whoops this was a mistake
Please fill in PR template's "Example entry for configuration.yaml" section, in your case, maybe template and script
I think I have done this now?
Please add unit test, home-assistant/tests/helpers/test_script.py
Added
Please fix the link to your document PR (remove the space between # and issue number)
Done

Thanks for your patience walking me through my first home-assistant PR

awarecan · 2018-08-06T02:11:51Z

Regarding Example entry for configuration.yaml

new flag 'proceed' to 'true' in the config.

I want to see the "example config" to demo how to set proceed

Wait Template script to use a timer to continue running the script

I want to see a example to demo "Wait Template script to use a timer"

It should be something similar with your unit test case, but write it in yaml format.

awarecan · 2018-08-06T03:04:42Z

homeassistant/const.py

 CONF_PENDING_TIME = 'pending_time'
 CONF_PIN = 'pin'
 CONF_PLATFORM = 'platform'
+CONF_PROCEED = 'proceed'


proceed is not self-explain to me. How about continue_on_timeout

And it should resident in helpers/script.py together with CONF_WAIT_TEMPLATE.

awarecan · 2018-08-06T03:08:15Z

homeassistant/helpers/script.py

                    self.hass.async_add_job(self._change_listener)

+                # Allow proceed to be false by default
+                proceed = False


No need declare here, should put it inside if CONF_TIMEOUT

awarecan · 2018-08-06T03:08:55Z

homeassistant/helpers/script.py

+                    # Check if we want to proceed to execute
+                    # the script after the timeout
+                    # or exit as the default behaviour
+                    if proceed:


if action.get(CONF_PROCEED, False):

awarecan · 2018-08-06T03:14:07Z

homeassistant/helpers/script.py

+                    # Check if we want to proceed to execute
+                    # the script after the timeout
+                    # or exit as the default behaviour
+                    if proceed:


Actually, if I were you, I will extend self._async_set_timeout method to accept the 3rd parameter as the callback method when timeout occurred. If the 3rd parameter is None, then keep current logic. If we want to continue run after timeout, we can simply pass in async_script_wait.

…nc_set_timeout

lhovo · 2018-08-06T15:11:05Z

Thanks, I was battling with the correct word to use.
I was originally trying to stay away from 'continue' as it's a key word in programming.

I have moved the "if CONF_CONTINUE" inside the _async_set_timeout.
It's passing the tests presently, but I would prefer to integrate it with my HA to test it some more to ensure the logic is 100% correct (assuming the test is correct, it should be fine)

If your happy with the change I will update the supporting docs with the new variable name

I will endeavour to do this tomorrow

awarecan

LGTM, doc need to change reflect latest change

lhovo · 2018-08-07T23:51:54Z

Not convinced the code is working as expected, when running on my home system.
Will do more testing soon, in the meantime I'm going to mark this as WIP

amelchio · 2018-08-09T05:36:49Z

I think we should consider for this to be the default.

lhovo · 2018-08-09T11:13:47Z

I would love for this to be the default, though that would make it a breaking change.

balloob · 2018-08-09T13:49:07Z

I agree with @amelchio that this should be the default behavior.

lhovo · 2018-08-12T11:01:21Z

I have spent the last week testing the present code and it is working as expected on my system

lhovo · 2018-08-12T11:04:22Z

As I have only just started to use Home Assistant and contribute back,
I am unsure on the steps required to change the default behaviour.

I can change the code and update the docs, is there anything else that needs to be done?
Like a vote between core developers?

amelchio · 2018-08-12T12:43:19Z

Having @balloob on board is enough :)

You tag the PR with "breaking change" and add a small note at the top describing the incompatibility. This note is copied to the release note and that's that.

…meout

lhovo · 2018-08-12T13:21:18Z

Either I am missing how to add a label, or I don't have permission.
Could someone add the breaking-change label for me?

Thanks

balloob · 2018-08-13T09:23:43Z

🎉 thanks for the great contribution!

lhovo · 2018-08-13T12:42:16Z

Thanks for being so inviting to your project!

…t#15836) * Adding new feature to allow a wait template to run the remainer of the script on timeout * Styling changes * Fixing file permissions, adding test for new code * changed variable name, refactored script to pass information into async_set_timeout * Changing the default behaviour to continue to run the script after timeout

Adding new feature to allow a wait template to run the remainer of th…

bdf03b3

…e script on timeout

lhovo requested a review from a team as a code owner August 5, 2018 02:45

homeassistant added cla-needed core small-pr PRs with less than 30 lines. labels Aug 5, 2018

ghost added the in progress label Aug 5, 2018

houndci-bot reviewed Aug 5, 2018

View reviewed changes

Styling changes

da38e86

homeassistant added cla-needed cla-signed and removed cla-needed labels Aug 5, 2018

fabaff changed the title ~~New feature: Allow wait template to run the remainder of the script~~ Allow wait template to run the remainder of the script Aug 5, 2018

lhovo mentioned this pull request Aug 5, 2018

Allow wait template to run the remainder of the script home-assistant/home-assistant.io#5954

Merged

2 tasks

Fixing file permissions, adding test for new code

91ce7af

homeassistant added the cla-signed label Aug 5, 2018

awarecan reviewed Aug 6, 2018

View reviewed changes

changed variable name, refactored script to pass information into asy…

1a56021

…nc_set_timeout

awarecan approved these changes Aug 7, 2018

View reviewed changes

lhovo changed the title ~~Allow wait template to run the remainder of the script~~ WIP Allow wait template to run the remainder of the script Aug 7, 2018

Changing the default behaviour to continue to run the script after ti…

c4c5799

…meout

homeassistant added the cla-signed label Aug 12, 2018

lhovo changed the title ~~WIP Allow wait template to run the remainder of the script~~ Allow wait template to run the remainder of the script Aug 12, 2018

MartinHjelmare added the breaking-change label Aug 12, 2018

balloob merged commit 6aee535 into home-assistant:dev Aug 13, 2018

ghost removed the in progress label Aug 13, 2018

balloob mentioned this pull request Aug 29, 2018

0.77 #16256

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

Uh oh!

Conversation

lhovo commented Aug 5, 2018 • edited by balloob 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!

homeassistant commented Aug 5, 2018

Uh oh!

houndci-bot Aug 5, 2018

Choose a reason for hiding this comment

Uh oh!

houndci-bot Aug 5, 2018

Choose a reason for hiding this comment

Uh oh!

homeassistant commented Aug 5, 2018

Uh oh!

awarecan commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhovo commented Aug 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awarecan commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awarecan Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

awarecan Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

awarecan Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

awarecan Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

lhovo commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awarecan left a comment

Choose a reason for hiding this comment

Uh oh!

lhovo commented Aug 7, 2018

Uh oh!

amelchio commented Aug 9, 2018

Uh oh!

lhovo commented Aug 9, 2018

Uh oh!

balloob commented Aug 9, 2018

Uh oh!

lhovo commented Aug 12, 2018

Uh oh!

lhovo commented Aug 12, 2018

Uh oh!

amelchio commented Aug 12, 2018

Uh oh!

lhovo commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balloob commented Aug 13, 2018

Uh oh!

lhovo commented Aug 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lhovo commented Aug 5, 2018 •

edited by balloob

Loading

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

awarecan commented Aug 5, 2018 •

edited

Loading

lhovo commented Aug 5, 2018 •

edited

Loading

awarecan commented Aug 6, 2018 •

edited

Loading

lhovo commented Aug 6, 2018 •

edited

Loading

lhovo commented Aug 12, 2018 •

edited

Loading