Skip to content

Add support for "alias" in device, device_condition, and conditions#40772

Closed
donkawechico wants to merge 2 commits intohome-assistant:devfrom
donkawechico:allow_comments_for_action_steps
Closed

Add support for "alias" in device, device_condition, and conditions#40772
donkawechico wants to merge 2 commits intohome-assistant:devfrom
donkawechico:allow_comments_for_action_steps

Conversation

@donkawechico
Copy link
Contributor

@donkawechico donkawechico commented Sep 30, 2020

Proposed change

See: WTH can’t we annotate sequence steps for logging?.

The backend already has the alias field to mostly achieve this WTH. But alias is not supported for all action steps.

This PR:

  • Adds alias to the following action step type schemas:
    • device
    • scene
    • condition (all -- "numeric_state", "state", "sun", etc..)
  • Consolidates duplicate calls to self._log("Executing step ... into a common method
  • Adds tests to ensure that all action schemas have alias (in case more are ever added)

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

Example entry for configuration.yaml:

# Example script.yaml
test_alias:
  alias: Test Alias
  sequence:
  - type: turn_off
    device_id: 401d32d1bfd440f0b887f02d2b8d5c66
    entity_id: light.chandelier
    domain: light
    alias: TURN OFF CHANDELIER
  - condition: state
    entity_id: light.chandelier
    state: 'off'
    alias: LEAVE SCRIPT IF CHANDELIER DIDNT TURN ON
  - scene: scene.warm
    alias: ACTUALLY FORGET THE CHANDELIER JUST MAKE IT NICE
  mode: single
  # ...

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.
  • 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:

@donkawechico donkawechico force-pushed the allow_comments_for_action_steps branch 5 times, most recently from 0f5b040 to 2939b94 Compare October 1, 2020 22:13
@donkawechico donkawechico marked this pull request as ready for review October 1, 2020 22:31
@donkawechico donkawechico force-pushed the allow_comments_for_action_steps branch from 2939b94 to 801b27e Compare October 1, 2020 22:48
@donkawechico donkawechico changed the title Optional "comment" field for action steps that gets appended to log Optional "description" field for action steps. Used for logs, readability. Oct 1, 2020
@donkawechico donkawechico changed the title Optional "description" field for action steps. Used for logs, readability. Optional "description" field for action steps, use in logs. Oct 1, 2020
@balloob
Copy link
Member

balloob commented Oct 2, 2020

We already allow setting an alias on each action step and that will be used in the log.

self._script.last_action = self._action.get(CONF_ALIAS, "call service")
self._log("Executing step %s", self._script.last_action)

What would this PR add that is not already there?

@donkawechico
Copy link
Contributor Author

donkawechico commented Oct 2, 2020

We already allow setting an alias on each action step and that will be used in the log. What would this PR add that is not already there?

A permanent record of my idiocy in git history? 🤣 How on earth did I miss that.

That said, looks, like alias is missing in DEVICE, DEVICE_CONDITION, and all of the CONDITION subschemas.

Guess I'll change this PR to be one that completes the support of alias, and also adds tests to ensure that all action step schemas support it going forward. Then I'll shift toward adding alias to "UI Mode" in the step editors.

ooof. Can we just keep this one between us?

@donkawechico donkawechico marked this pull request as draft October 2, 2020 18:03
@donkawechico donkawechico changed the title Optional "description" field for action steps, use in logs. Add support for "alias" in device, device_condition, and condition subschemas Oct 2, 2020
@donkawechico donkawechico force-pushed the allow_comments_for_action_steps branch from 3fec293 to 81093af Compare October 2, 2020 20:19
@donkawechico donkawechico marked this pull request as ready for review October 2, 2020 20:35
@donkawechico
Copy link
Contributor Author

@balloob (and/or others) I've converted the PR from adding unnecessary "description", to one that just adds alias to the steps that didn't have it yet.

@donkawechico donkawechico force-pushed the allow_comments_for_action_steps branch from 81093af to d9d4d15 Compare October 2, 2020 20:55
@donkawechico donkawechico changed the title Add support for "alias" in device, device_condition, and condition subschemas Add support for "alias" in device, device_condition, and conditions Oct 14, 2020
@donkawechico
Copy link
Contributor Author

Bump. The frontend just got collapsible panels that would be perfect for the project behind this PR (collapsing sequence steps into panels that are labeled with the alias).

def test_action_type_schemas_support_alias():
"""Test that all action type schemas allow alias field."""

def find_root_schema(node):
Copy link
Member

@balloob balloob Oct 15, 2020

Choose a reason for hiding this comment

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

Instead of this, we should embed a config for each notification and error out if we don't have a config for a condition in cv.ACTION_TYPE_SCHEMAS.

Comment on lines +342 to +343
if action_type == cv.SCRIPT_ACTION_CHECK_CONDITION:
continue
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the tests for conditions?

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants