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

Add tests for types and functions for type conversions in templates #100807

Merged
merged 27 commits into from Oct 25, 2023

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Sep 24, 2023

Proposed change

Jinja has native tests for checking if a value is a mapping, string, float, int, or number, but it is missing tests for other types, specifically non string iterables. To check if something is a list, according to https://stackoverflow.com/questions/11947325/how-to-test-for-a-list-in-jinja2 you have to write an, in my opinion, long and awkward check. Instead, we can add these convenience functions to make things easier for users. There is also no easy check for datetime which is a common enough type for such a test to be useful

We also added the ability to convert a value to a set or tuple. Offhand, I am not sure of the value of supporting converting to a tuple beyond giving users the ultimate flexibility, but a set is useful because you can easily perform intersection, issubset, and difference functions between two sets

Adds new functions to help template users convert inputs into a specific type:

  • set
  • tuple

Adds new tests to help template users tests for certain types:

  • list
  • set
  • tuple
  • string_like
  • datetime

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • 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

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • 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.

To help with the load of incoming pull requests:

@thomasloven
Copy link
Contributor

thomasloven commented Sep 24, 2023

There's a typo in the description.
It should be is_non_string_iterable, not is_non_iterable_string which I have hard imagining what that could even be...

@raman325
Copy link
Contributor Author

There's a typo in the description.

It should be is_non_string_iterable, not is_non_iterable_string which I have hard imagining what that could even be...

🤦‍♂️ thanks

@tdejneka
Copy link

Suggestion:

Given is_list and is_set, how about is_list_or_set instead of is_non_string_iterable?

Also does is_list handle tuples?

@raman325
Copy link
Contributor Author

raman325 commented Sep 25, 2023

is_list does not handle tuples. We could add is_tuple as well but is_non_string_iterable works for tuple in addition to set or list

@tdejneka
Copy link

I have seen tuples reported by Home Assistant's native typing but I can't recall encountering sets. Does native typing ever return a set? Attempting to define a set in the Template Editor like this results in an error:

{% set x = {"cat", "dog"} %}

Is there any reason they are implemented as filters instead of tests? The addition of tuple, list as tests would be consistent with the following built-in tests:

{{ {'qty': 56, 'color': 'red'} is mapping }}
{{ 5 is number }}
{{ 5 is integer }}
{{ 5.6 is float }}
{{ 'cat' is string }}
{{ false is boolean }}
{{ (1,2) is iterable and (1,2) is not string }}
{{ 'cat' is iterable and 'cat' is not string }}

Screenshot_20230924-203447~2

@raman325
Copy link
Contributor Author

raman325 commented Sep 25, 2023

they're implemented as filters, functions, and tests, to be consistent with is_number

regarding sets, you create a set by doing set(1, 2) which is valid in Jinja. With that being said, the only example I have right now is when you get the identifiers or connections attribute from a device entry -> those are returned as sets

@tdejneka
Copy link

you create a set by doing set(1, 2) which is valid in Jinja

I couldn't get that to work in 2023.8.3. Does it work in 2023.9.0 or in the current dev version?

Screenshot_20230925-074341~2

@TheFes
Copy link
Contributor

TheFes commented Sep 25, 2023

Could is_datetime be added?

@raman325
Copy link
Contributor Author

raman325 commented Sep 25, 2023

I couldn't get that to work in 2023.8.3.

Not sure what I was doing when I said this but I can't reproduce so you are right. Regardless, identifiers and connections for a device entry are returned as sets

@tdejneka
Copy link

identifiers and connetions for a device entry are returned as sets

Understood.

For future reference, it would be useful if there was more support for sets in Home Assistant's implementation of Jinja2. At the moment there's not much one can do other than iterate through a set's values.

{% for i in device_attr('9bf07320ae9bc506dd6486d8071a0582', 'identifiers') %}
{{i}}
{% endfor %}

I can't confirm if useful set methods like difference, intersection, issubset, etc are supported because of the inability to define a set and the challenge of finding suitable sets, for testing, via device_attr.

Anyway, thanks for the addition of the useful filters/tests/functions.

@raman325
Copy link
Contributor Author

raman325 commented Sep 25, 2023

sorry, didn't realize I repeated myself about the device entries 😅

Regarding set support what did you have in mind? Do you want more flexibility for manipulating/accessing these two attributes, or are you suggesting sets should be in more places across the core?

thinking about this some more, sets are useful to convert lists into to find intersections, differences, etc. (as you mentioned). So I think we'd want to add a filter that converts a value into a set? From there we'd have to test if those functions work

@raman325
Copy link
Contributor Author

raman325 commented Sep 25, 2023

native set functions appear to work. Example: {{ device_attr('lock.front_door', 'identifiers').intersection(device_attr('lock.front_door', 'connections')) }} evaluates to set(), and if I do an intersection on itself, I get the full set back. So I think all we need is a filter/function to convert an iterable to a set

@raman325 raman325 changed the title Add template tests for types Add tests for types and functions for type conversions in templates Sep 26, 2023
Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Please update the PR description to explain why the new functionality is useful.

@home-assistant home-assistant bot marked this pull request as draft September 27, 2023 12:09
@home-assistant
Copy link

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

Learn more about our pull request process.

@raman325 raman325 marked this pull request as ready for review September 27, 2023 14:24
@emontnemery
Copy link
Contributor

@raman325 I was chatting with @edenhaus about why this PR adds functions, filters and tests to check the typing. Why do we need all three instead of just the tests?

@raman325
Copy link
Contributor Author

raman325 commented Sep 27, 2023

@raman325 I was chatting with @edenhaus about why this PR adds functions, filters and tests to check the typing. Why do we need all three instead of just the tests?

I suppose we don't... I was following the pattern for is_number. It would help to have some guidance on what should be added as a function vs a filter vs a test because right now the approach seems inconsistent.

Anyway, should I remove the filters and functions?

raman325 and others added 4 commits October 7, 2023 10:32
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
homeassistant/helpers/template.py Outdated Show resolved Hide resolved
homeassistant/helpers/template.py Outdated Show resolved Hide resolved
homeassistant/helpers/template.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft October 23, 2023 07:44
@raman325 raman325 marked this pull request as ready for review October 24, 2023 05:46
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Small doc improvements.

Otherwise fine to go

tests/helpers/test_template.py Outdated Show resolved Hide resolved
tests/helpers/test_template.py Outdated Show resolved Hide resolved
tests/helpers/test_template.py Outdated Show resolved Hide resolved
tests/helpers/test_template.py Outdated Show resolved Hide resolved
tests/helpers/test_template.py Outdated Show resolved Hide resolved
tests/helpers/test_template.py Outdated Show resolved Hide resolved
tests/helpers/test_template.py Outdated Show resolved Hide resolved
raman325 and others added 7 commits October 24, 2023 09:15
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Co-authored-by: Robert Resch <robert@resch.dev>
Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

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

Thanks @raman325 👍

@edenhaus edenhaus merged commit 35d18a9 into home-assistant:dev Oct 25, 2023
53 checks passed
@edenhaus edenhaus added the noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Oct 25, 2023
@raman325 raman325 deleted the tests branch October 25, 2023 13:26
@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed core new-feature noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants