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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Handle dict syntax in state trigger "for" #1725

Merged
merged 2 commits into from
Oct 5, 2018

Conversation

emlove
Copy link
Contributor

@emlove emlove commented Oct 2, 2018

Description

Our for configuration in our state trigger currently supports any of the following formats. This PR adds client handling to convert the dict format into string format so it can be rendered and edited in a single text field.

for: 30 # seconds
for: '2:15:30' # 2 hours, 15 minutes, 30 seconds
for:
  hours: 2
  minutes: 15
  seconds: 30

Questions

  1. Should we even support loading this syntax in the frontend? It'll only be present for converted automations. If not, should we deprecate this syntax, since it's functionally equivalent with the string syntax. It also seems confusing, since it appears very similar to the time interval trigger configuration, but behaves differently, since we're only declaring a delay time, instead of a periodic interval.
  2. If we do want to support this, we'll need to roll it out to numeric_state as well, so maybe there's somewhere better for this logic to live, or maybe it should even be converted in the backend view.

@ghost ghost assigned emlove Oct 2, 2018
@ghost ghost added the in progress label Oct 2, 2018
@balloob
Copy link
Member

balloob commented Oct 3, 2018

I think that converting to string format is good 👍

@balloob
Copy link
Member

balloob commented Oct 3, 2018

I can't remember why we support the dict format btw 🤔

@balloob
Copy link
Member

balloob commented Oct 3, 2018

Ah the implementation is that the dict format is just passed as keyword args to the timedelta constructor.

seconds = 0,
} = trgFor;
hours = hours.toString();
minutes = minutes.toString().padStart(2, '0');
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to add padStart polyfill to compatability.js

@balloob balloob merged commit 606a220 into master Oct 5, 2018
@ghost ghost removed the in progress label Oct 5, 2018
@balloob balloob deleted the trigger-for-handle-dict-syntax branch October 5, 2018 09:33
zsarnett pushed a commit to zsarnett/home-assistant-polymer that referenced this pull request Oct 13, 2018
* Handle dict syntax in state trigger "for"

* padStart polyfill
zsarnett pushed a commit to zsarnett/home-assistant-polymer that referenced this pull request Oct 13, 2018
* Handle dict syntax in state trigger "for"

* padStart polyfill
zsarnett pushed a commit to zsarnett/home-assistant-polymer that referenced this pull request Oct 14, 2018
* Handle dict syntax in state trigger "for"

* padStart polyfill
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 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.

None yet

4 participants