Split weekday from time condition into a separate condition

[Misiu]

Context

I'm trying to add weekday selection to the automation editor (home-assistant/frontend#3833). I had most of the work done, but while building thinks I got feeling that time and weekdays should be split as two separate conditions.

Proposal

I'm proposing to remove weekday from time condition and create a separate condition.
So instead:

condition:
  condition: time
  after: '15:00:00'
  before: '02:00:00'
  weekday:
    - mon
    - wed
    - fri

you will write:

condition:
  - condition: time
    after: '15:00:00'
    before: '02:00:00'
  - condition: weekday
    days:
      - mon
      - wed
      - fri

Consequences

This will allow me to split a single complex condition into two:

to:

[Misiu]

Looking at source I noticed that time condition is specified in https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/condition.py#L394-L448
Any other places except config_validation.py and tests?

If we agree on naming I can try to hack something out.

What do you think about two options?

condition:
  - condition: time
    after: '15:00:00'
    before: '02:00:00'
  - condition: weekday
    days:
      - mon
      - wed
      - fri

or:

condition:
  - condition: time
    after: '15:00:00'
    before: '02:00:00'
  - condition: weekday
    mon: true
    wed: true
    fri: true
    sat: false

in the second example, all the days would default to false.

I think that to keep backward compatibility we can leave weekday as part of time condition, but mark it as deprecated and remove it in one of upcoming releases.
The new condition would work independently. This way we will have backward compatibility and new feature.

[Santobert]

Personally, I like this one:

- condition: weekday
  days:
    - mon
    - wed
    - fri

[Misiu]

@Santobert thanks for the comment. It will be easier for me with this config :)
I'll try to hack something. As I wrote I'll create a separate condition to have backward compatibility.
Are helpers/condition.py the correct starting point?

[Swamp-Ig]

Just wondering, would something like:

- condition: time
  days:
    - weekday

or:

- condition: time
  days:
    - weekend

or:

- condition: time
  days:
    - mon
    - thu

be better?

[Misiu]

@Swamp-Ig this is already possible. You can add weekday as part of time condition (https://www.home-assistant.io/docs/scripts/conditions/#time-condition)
My intention is to create a separate condition for weekdays only. This was recommended by @bramkragten in home-assistant/frontend#3833
When this will be ready I'll update the automation editor to allow using this new condition.

[Misiu]

I've created PR home-assistant/core#32848 but I have some formatting issues with flak8 and black. I can't get both rules to pass.
Anyone can help?

[Misiu]

docs PR got approved.
Now I'm waiting for Core and Frontend PR review/approval.
@Santobert I got no votes against this change, so I think it could be merged.
What is the best way to contact the devs and ask for a review? I don't want to mention anyone in comments to avoid bans :/

[Santobert]

What is the best way to contact the devs and ask for a review? I don't want to mention anyone in comments to avoid bans :/

Most of them use discord, so try it there. There are currently a lot of PRs pending, so be patient :)

[Misiu]

@Santobert thanks for the tip 😉
I'll wait a couple of days more and then ask on Discord.

[MartinHjelmare]

I don't see any motivation why we should split the time weekday condition to a separate condition. Only one of after, before or weekday is required, so I think we're already able to combine two conditions, one with only weekday. Doesn't that work?

[Misiu]

@MartinHjelmare yes that's true, but please take a look at home-assistant/frontend#3833.
@bramkragten give green light to this.
My main motivation was to create an UI editor. I think the condition block should be as small as possible.
This PR creates second condition. I want to leave the old behavior for some time and remove it after couple of releases.

[MartinHjelmare]

We normally don't take the frontend into consideration when making backend architecture decisions.

Why can't the frontend have a selector that decides which one of the three possible conditions to add?

[fjufju]

Needed in automation editor.
Can this be done?

[emontnemery]

Why is it needed in automation editor?

[bramkragten]

I agree that it is already possible, I just think it might be nice to keep the frontend and yaml config aligned, but don't think it would be worth the breaking change.

[Misiu]

Recent discussion on the forum: https://community.home-assistant.io/t/condition-day-was-schedule-trigger-type/219551

[Misiu]

I agree that it is already possible, I just think it might be nice to keep the frontend and yaml config aligned, but don't think it would be worth the breaking change.

What do you think about this prototype:

More details here: home-assistant/frontend#3833 (comment)

[emontnemery]

@Misiu The prototype looks nice!

[emontnemery]

I think this issue can be closed: https://www.home-assistant.io/blog/#automation-editor-has-now-weekday-support ?

[frenck]

Yes, this is now there.