Skip to content

Rename time trigger 'after' to 'at'#7846

Merged
balloob merged 1 commit into
home-assistant:devfrom
emlove:time-trigger-rename
Jun 2, 2017
Merged

Rename time trigger 'after' to 'at'#7846
balloob merged 1 commit into
home-assistant:devfrom
emlove:time-trigger-rename

Conversation

@emlove
Copy link
Copy Markdown
Contributor

@emlove emlove commented Jun 1, 2017

Description:

This PR renames the time trigger 'after' option to 'at'.

In #7556, implemented with #7651, we decided it's too confusing to try and force the trigger configuration names to match the condition names. Similarly, 'after' as a trigger name isn't exactly intuitive. This PR proposes to change it to 'at'. I'm also open to other ideas for the name if someone has a better idea, but it should at least be better than 'after'.

CC @amelchio

Breaking change
For time triggers, after is deprecated and replaced with at.

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

Example entry for configuration.yaml (if applicable):

automation:
  trigger:
    platform: time
    at: '15:32:00'

@mention-bot
Copy link
Copy Markdown

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

@amelchio
Copy link
Copy Markdown
Contributor

amelchio commented Jun 1, 2017

I agree, it's confusing. Typing after makes me wonder "how long after?"

If we want to clean up all of these, maybe #7170 should be considered at the same time?

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 1, 2017

Ah, good catch. I agree with that as well. I'll try and get out a PR today, since it would be nice if we could get all of our trigger refactoring into 0.46. Should just be a matter of fixing here: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/condition.py#L169-L173

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 1, 2017

Can we think of a use case that would require the old logic? I'm wondering if we should also include the keywords min and max that replicate the existing logic, making both options available. I can't think of anything off the top of my head, but when it comes to floating points sometimes it's the desired behavior and not cleanly replicated otherwise.

@amelchio
Copy link
Copy Markdown
Contributor

amelchio commented Jun 1, 2017

I have that same feeling but cannot think of an example off the top of my head.

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 1, 2017

Opened #7857 for discussion.

@balloob balloob merged commit cf42303 into home-assistant:dev Jun 2, 2017
@balloob balloob mentioned this pull request Jun 2, 2017
@emlove emlove deleted the time-trigger-rename branch June 2, 2017 15:20
@RiRomain
Copy link
Copy Markdown
Contributor

RiRomain commented Jun 2, 2017

Did you leave backward compatibility? Supporting at and after would be nice, at least for a few revision, this will break too much configuration and to handle both keyword it mustn't be too hard, or?

@balloob
Copy link
Copy Markdown
Member

balloob commented Jun 2, 2017

Read either the PR description or the code…

@emlove
Copy link
Copy Markdown
Contributor Author

emlove commented Jun 2, 2017

@RiRomain Yes, there is currently backward compatibility. A warning will be issued in the logs if the old name is detected.

@RiRomain
Copy link
Copy Markdown
Contributor

RiRomain commented Jun 2, 2017

@balloob I did, didn't see any mention about a backward compatibility but rather a "breaking change".

@armills Ok, thanks for the reply and for the change, it does make the trigger simpler to apprehend :-)

@arsaboo
Copy link
Copy Markdown
Contributor

arsaboo commented Jun 2, 2017

@RiRomain It is mentioned right the description

For time triggers, after is deprecated and replaced with at.

jnewland added a commit to jnewland/ha-config that referenced this pull request Jun 4, 2017
jnewland added a commit to jnewland/ha-config that referenced this pull request Jun 4, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
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.

8 participants