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

Provide stub config for entity-filter #8121

Merged
merged 3 commits into from Feb 7, 2021

Conversation

spacegaier
Copy link
Member

@spacegaier spacegaier commented Jan 9, 2021

Breaking change

Proposed change

Void in case #7637 gets merged soon.

Currently, if you add an entity-filter you are greeted by the YAML editor in an error state, because mandatory config keys are missing. With this PR we now provide something meaningful as a starting point.

grafik

Type of change

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

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

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

@thomasloven
Copy link
Contributor

thomasloven commented Jan 9, 2021

sun.sun isn't guaranteed to exist. What happens if it doesn't?

In #7637 I went for the simplest possible stub: { entities: [], state_filter: [] }

@spacegaier
Copy link
Member Author

Nothing really happens. It is just as if the entity was filtered.
Originally I had an empty entity list, but then I decided to go for a more complete stub, to have a more ready to use basis (otherwise you have to remove the [] and then add a new line, indent, add the - entity, ... Nothing major, but all extra steps that I wanted to free the user from.

Your version for sure makes sense as the stub for a real GUI editor, since there the user does not need to thing about any YAML structure.

@bramkragten
Copy link
Member

The stub should always be the most simple config, so I think the suggestion from Thomas works.

@spacegaier
Copy link
Member Author

If we provide no entities, then the user is greeted by this. Feels weird.

image

  1. Starting with an error state
  2. The user then has to remove the empty arrays and convert to actual entries (error / confusion risk)

But if Thomas' PR gets merged soon-ish, then this here is not a hill I am willing to die on 😉.

@thomasloven
Copy link
Contributor

If we provide no entities, then the user is greeted by this. Feels weird.

That's a good point. But maybe we should have it pick a few entities at random instead? Like the entities card does.

@spacegaier
Copy link
Member Author

If we provide no entities, then the user is greeted by this. Feels weird.

That's a good point. But maybe we should have it pick a few entities at random instead? Like the entities card does.

That was my initial thought as well, but wasn't sure what filter states to choose? That's why I went with sun which usually exists, although its state filter is only hit during half of the day. So double not ideal indeed.

We could just select three random binary_sensors and set card filter state to "on". There is a decent change at least one of the randomly picked sensors is "on" so that the user sees an entity in the card preview, although that is nice to have.

@bramkragten
Copy link
Member

bramkragten commented Jan 27, 2021

You could pick the current state of the first entity as the filter?

@bramkragten bramkragten merged commit 01df01c into home-assistant:dev Feb 7, 2021
spacegaier added a commit that referenced this pull request Feb 7, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2021
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