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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop script if sub-script stops or aborts #71195

Merged
merged 1 commit into from May 2, 2022
Merged

Conversation

emontnemery
Copy link
Contributor

Proposed change

Stop script if sub-script stops or aborts

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@emontnemery emontnemery added this to the 2022.5.0 milestone May 2, 2022
@emontnemery emontnemery requested a review from a team as a code owner May 2, 2022 12:03
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @emontnemery 馃憤

@frenck frenck merged commit f6c2fb0 into dev May 2, 2022
@frenck frenck deleted the script_abort_subscript branch May 2, 2022 13:00
@KevinCathcart
Copy link
Contributor

Is this not a breaking change? Certainly some people could be accidentally or even deliberately relying on existing aborts not bubbling up.

And in one case I think the current behavior is documented:

The condition action only stops executing the current sequence block. When it is used inside a repeat action, only the current iteration of the repeat loop will stop. When it is used inside a choose action, only the actions within that choose will stop.

But the condition action looks to work by raising _AbortScript. With this change that now bubbles upward?

Perhaps I am missing something though. I'm only reading the code, not actually testing this change out.

@frenck
Copy link
Member

frenck commented May 2, 2022

Perhaps I am missing something though. I'm only reading the code, not actually testing this change out.

That doesn't rely on aborting.

image

Those are condition errors, which are caught to break the loop.

@KevinCathcart
Copy link
Contributor

KevinCathcart commented May 2, 2022

But I'm talking about condition as an action type within a choose branch or within a loop. https://www.home-assistant.io/docs/scripts/#test-a-condition

The condition action step looks to be turning failed conditions or ConditionErrors into an abort:

async def _async_condition_step(self):
"""Test if condition is matching."""
self._script.last_action = self._action.get(
CONF_ALIAS, self._action[CONF_CONDITION]
)
cond = await self._async_get_condition(self._action)
try:
trace_element = trace_stack_top(trace_stack_cv)
if trace_element:
trace_element.reuse_by_child = True
check = cond(self._hass, self._variables)
except exceptions.ConditionError as ex:
_LOGGER.warning("Error in 'condition' evaluation:\n%s", ex)
check = False
self._log("Test condition %s: %s", self._script.last_action, check)
trace_update_result(result=check)
if not check:
raise _AbortScript

@frenck
Copy link
Member

frenck commented May 2, 2022

Meh that looks like a totally different bug in that case.
(As the repeat loops now check for a ConditionError that can never been thrown as it seems).

@KevinCathcart
Copy link
Contributor

KevinCathcart commented May 2, 2022

Are the conditions of the loop actually treated as action steps? Or just evaluated more directly?

In any case be 100% clear about the case I am referring to, I'm talking about scripts like this one which should make the left light blink at twice the rate of the right light. I've hand typed this, so i may have messed something up.

      - repeat:
          count: 16
          sequence:
            - delay: 2
            - service: light.toggle
              target:
                entity_id: "light.left"
            - alias: ignore odd indexes for the right light
              condition: "{{ repeat.index % 2 == 0 }}"
            - service: light.toggle
              target:
                entity_id: "light.right"

The condition failing is like a continue statement here, at least if the docs are accurate.

But the same concept also applies to choose statements. where the conditional failing would act sort-of like a break statement within a switch in C like languages, except that choose lacks fall-though.

@emontnemery
Copy link
Contributor Author

@KevinCathcart #71235 makes sure a failing condition in a sub-script does not bubble up and stops the parent.

@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2022
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.

stop action only stops active sequence, not entire script
5 participants