core icon indicating copy to clipboard operation
core copied to clipboard

Add has_value function/test to Jinja2 template

Open ehendrix23 opened this issue 2 years ago • 18 comments

Proposed change

Add a function and test to determine if an entity is available ('unknown' or 'unavailable).

Checking if an entity is available is something done regularly within template sensors.

Currently this is done through: states('<entity>') not in (('unknown', 'unavailable')) And often when multiple entities are to be checked combined with AND (if at least one should be available) or OR (if none can be unavailable). {{ states('<entity1>') not in (('unknown', 'unavailable')) or states('<entity2>') not in (('unknown', 'unavailable')) or states('<entity3>') not in (('unknown', 'unavailable')) }}

With this change this can be simplified to: {{ has_value('<entity1>') or has_value('<entity2>') or has_value('<entity3>')

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [X] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes https://community.home-assistant.io/t/wth-we-dont-have-a-is-available-filter/468986
  • This PR is related to issue:
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/24360

Checklist

  • [X] The code change is tested and works locally.
  • [X] Local tests pass. Your PR cannot be merged unless tests pass
  • [X] There is no commented out code in this PR.
  • [X] I have followed the development checklist
  • [X] The code has been formatted using Black (black --fast homeassistant tests)
  • [X] 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.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] 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:

ehendrix23 avatar Oct 03 '22 18:10 ehendrix23

Saw this idea in WTH: https://community.home-assistant.io/t/wth-we-dont-have-a-is-available-filter/468986 and agreed, I have a lot if items that would be a lot more simple through this.

ehendrix23 avatar Oct 03 '22 19:10 ehendrix23

Anything more that has to be done on this PR before it can be merged?

Thx.

ehendrix23 avatar Nov 15 '22 14:11 ehendrix23

@frenck Anything further required for this PR?

ehendrix23 avatar Dec 05 '22 22:12 ehendrix23

@frenck @balloob What can we do to get this completed? Any requested changes have been completed.

ehendrix23 avatar Dec 21 '22 20:12 ehendrix23

Hello, I like this PR. I believe this method should be added.

It solves a clear use case and is a huge simplification over the current equivalent solution, which is not very intuitive (for new users) and an eyesore in otherwise clean yaml syntax of a template sensor or automation.

The name to use for the method was a lenghty discussion and is_nominal seemed like a well accepted term, despite its theoretical shortcomings.

Here is another reason why I think this PR should be considered for eventual merge:

  • The need for this function was discussed in an WTH by quite a few users: https://community.home-assistant.io/t/wth-we-dont-have-a-is-available-filter/468986
  • Multiple members of the community concluded on its implementation details
  • A solution was developed, published and peer reviewed

It is an excellent example of collaboration and consensus in an open source project and deserves that we adhere to a democratic process.

ThomDietrich avatar Jan 26 '23 16:01 ThomDietrich

I will take it into the core meeting to see if there is support from other maintainers. From my end, I'm against the PR for the above reasoning.

Marking the PR as a draft in the mean time.

../Frenck

frenck avatar Jan 26 '23 17:01 frenck

I also think this is something so simple and so useful, that not being in core is a shame! Also it's not something that is possible to implement via extensions. And since jinja template includes is not possible, something like this, if not on core, it's impossible to add.

This is my request to make it core! Please consider!

Thank you

soloam avatar Jan 26 '23 17:01 soloam

Let's stop adding comments to show "I want this", we have the forums for that. Let's not bring the whole forums topic into this PR :)

As said, I'll take it with me for discussion.

../Frenck

frenck avatar Jan 26 '23 17:01 frenck

So, I've taken it into a core meeting, and we agreed that the naming isn't acceptable to us.

We have also discussed alternatives and agreed we are willing to accept this feature if the name of the method is changed to has_value.

The reasoning behind has_value is: Both unknown & unavailable states, are not values originating from the device or service. "Unknown" originates from a NoneType value, while when "unavailable" it would be impossible to have a value at all. For both cases, has_value seems fitting and logical.

../Frenck

frenck avatar Feb 02 '23 14:02 frenck

We have also discussed alternatives and agreed we are willing to accept this feature if the name of the method is changed to has_value.

Changed to recommended has_value. Tests completed successfully and working on my local copy. Documentation has also been updated to name has_value.

ehendrix23 avatar Feb 08 '23 18:02 ehendrix23

I've marked this PR, as changes are requested that need to be processed. Please un-draft it once it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

../Frenck

frenck avatar Feb 08 '23 22:02 frenck

@ehendrix23 It looks like the upcoming release will have quite a bit of changes and additions to the template engine. It would be cool if we could include this. Could you look at the comments above?

frenck avatar Mar 09 '23 20:03 frenck

@ehendrix23 It looks like the upcoming release will have quite a bit of changes and additions to the template engine. It would be cool if we could include this. Could you look at the comments above?

Sorry, been extremely busy lately with work and family. Will put this as high priority to get in.

Provided comment on list why I had it in there. If after my comment you still feel it should be removed I will remove it ASAP. :-)

For the None check, I understand what you mean but checking that the entity exist and is not just "unavailable".

Just a quick check though. Once an entity appeared in HASS, will entity.state then return STATE_UNKNOWN if on next restart of HASS for example the integration does not start? If so then I indeed don't see a reason for that check since the main purpose is to check state of entities that at some point existed and not removed from the registry.

ehendrix23 avatar Mar 10 '23 00:03 ehendrix23

Let me trow my 2 cents on this! Since this was originated from my WTH request. I don't think you should ditch the unknown status, the main reason for this, on my point of view, is to quickly and easily filter entities that don't have a valid value! For me a entity with, unknown, undefined, unavailable is something that is not valid!

Thank you for your work and time on this

soloam avatar Mar 10 '23 01:03 soloam

unknown status

Nobody said that?

For me a entity with, unknown, undefined, unavailable is something that is not valid!

Correct, however, undefined must raise errors and not considered normal.

../Frenck

frenck avatar Mar 13 '23 09:03 frenck

unknown status

Nobody said that?

For me a entity with, unknown, undefined, unavailable is something that is not valid!

Correct, however, undefined must raise errors and not considered normal.

../Frenck

But the idea with this function as well is to be able to prevent a template to raise an error when an entity does not have a value, this would then include when the entity is undefined due to issues with the integration.

ehendrix23 avatar Mar 13 '23 13:03 ehendrix23

When an entity doesn't have a value it will result in an state: "unknown".

Undefined entities are errors. Even when an integration is not loaded, the entity is still there an in the registry (and thus has an unknown state).

Undefined is an error.

frenck avatar Mar 13 '23 13:03 frenck

Changes have been made removing checking for None and removing option to provide a list.

ehendrix23 avatar Mar 13 '23 13:03 ehendrix23

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Mar 26 '23 17:03 home-assistant[bot]