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 more strict type tests #824

Merged
merged 1 commit into from Oct 10, 2019
Merged

Add more strict type tests #824

merged 1 commit into from Oct 10, 2019

Conversation

@dagwieers
Copy link
Contributor

dagwieers commented Mar 17, 2018

This PR adds a few more type-related tests.

  • boolean
    Testing if an object is a boolean required 2 tests.

      {% if result.value is boolean %}
    
  • false
    Make this similar to testing none value, not requiring sameas

      {% if result.value is false %}
    
  • true
    Make this similar to testing none value, not requiring sameas

      {% if result.value is true %}
    
  • integer
    The existing 'number' test does not make a distinction between
    integer, float or even booleans

      {% if result.value is integer %}
    
  • float
    The existing 'number' test does not make a distinction between
    integer, float or even booleans

      {% if result.value is float %}
    
  • list
    The existing 'sequence' or 'iterable test does not make a distinction
    between strings, lists or mappings. Even 'iterable' does not help here.

      {% if result.value is list %}
    

This brings the same convenience as:

  • none

      {% if result.value is none %}
    
  • string

      {% if result.value is string %}
    
  • mapping

      {% if result.value is mapping %}
    

For the original Jinja2 use-case where values eventually turn into strings anyway, type-checking is less of an issue, however in Ansible or other projects that use Jinja2 for more than web-templating this is more important.

We see people turning booleans into strings to compare with 'True' or 'False'. Or doing very weird things to determine what is a string, or a list. If you're not careful you end up with a character, instead of the first element.

You can see the effect of testing different types in Jinja including these new tests:
https://github.com/dagwieers/ansible/blob/jinja2-type-tests/test/integration/targets/jinja2_tests/tasks/main.yml

PS This PR also has a workaround for a small doc issue breaking Travis testing.

@abadger

This comment has been minimized.

Copy link
Contributor

abadger commented May 21, 2018

list is not a good name as it is not detecting whether the data type is a python list. What's really meant is test_nonstring_sequence but that might be a bit cumbersome to type. In other APIs I've written, I've made selecting whether to exclude string types a parameter of the function but I don't think that jinja tests take parameters from the user so that's not an option here.

@artheus

This comment has been minimized.

Copy link

artheus commented Nov 20, 2018

Related to Issue #358
This will be a great feature in jinja.
I approve 👍

@artheus

This comment has been minimized.

Copy link

artheus commented Nov 20, 2018

list is not a good name as it is not detecting whether the data type is a python list. What's really meant is test_nonstring_sequence but that might be a bit cumbersome to type. In other APIs I've written, I've made selecting whether to exclude string types a parameter of the function but I don't think that jinja tests take parameters from the user so that's not an option here.

What about array? nonstring-sequence would also be true for dicts, which I am guessing is not exactly what we'd want here.

@abadger

This comment has been minimized.

Copy link
Contributor

abadger commented Nov 20, 2018

  • array is a better name. Although strings are arrays, array doesn't clash with the builtin python list type.
  • nonstring-sequence would not be true for dicts. (dicts are iterable but not indexable. Thus they are not sequences).
  • Note that your question as to whether dicts are wanted reminds me that sometimes dicts are wanted and other times they are not. I almost always check whether an object is "iterable and not string" rather than "sequence and not string". However, that is mostly for the following:
  • Sequences do not contain sets and frozensets. To capture those one has to test whether an object is an iterable.
  • In most of the instances I need a check for something like this (I actually can't think of a time when I chose to check for a sequence instead of an iterable...), it is because I want to determine if I can loop over the data rather than whether I can reference a specific index in the data. So I accept that dicts will be included (usually fine, just not the format that I'd expect the user to be sending me) or I check for Mappings before iterability if the particular piece of code cares.
@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 20, 2018

All of this seems very specific to Ansible. I've never had to do these sorts of type checks in templates, or in most Python code in general. Maybe the issue with inconsistent data that users have to check is something that needs to be addressed by Ansible?

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 20, 2018

For the boolean, why can't {% if value %} and {% if not value %} be used? In what case do we need to know that it's specifically a boolean, and not just expect to get a boolean for that value?

For "sequence not string", you can already do this with {% if value is sequence and value is not string %}. If you expect either a string or an iterable, which seems like the most common case, then you can just swap the statement and use

{% if value is string %}
  it's one thing
{% else %}
  it's many things
{% endif %}

or

{% if value is string %}{% set value = [value] %}{% endif %}
{% for item in value %}one code path{% endfor %}

For int vs float as opposed to "number", why does it matter at the template level what type these are? If you absolutely need one or the other, then use |int, |float, or |round.

@flexferrum

This comment has been minimized.

Copy link
Contributor

flexferrum commented Nov 20, 2018

Why not to add type_id virtual field to each value? This field can represent the type name of the value and can be tested in the way most suitable for template author.

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 20, 2018

Testing against strings sounds really error-prone.

@flexferrum

This comment has been minimized.

Copy link
Contributor

flexferrum commented Nov 20, 2018

From the one side 'yes'. From the other... According this discussion current set of type testers can't guarantee the strong types distinguishing from each other. Another point: this feature will allow to authors decribe theirs own types came outside of template engine. Personally I have faced with the problem: I reflect variant type into Jinja template but haven't got the "standard" way to describe the currently reflected particular type from the variant cases.

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 20, 2018

If you really want to test based on strings, you can already do that with value.__class__.__name__, or an application can add a test for that. But it's not something I'm willing to add to Jinja.

This whole thing seems like something that should not have to be done in templates, but the problem is that there are already some type tests so it's not clear what was intended and if it's ok to keep piling more in.

@flexferrum

This comment has been minimized.

Copy link
Contributor

flexferrum commented Nov 20, 2018

If you really want to test based on strings, you can already do that with value.class.name, or an application can add a test for that. But it's not something I'm willing to add to Jinja.

Yes. You right. It's better to implement via extension/application-specific test.

@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Nov 20, 2018

All of this seems very specific to Ansible. I've never had to do these sorts of type checks in templates, or in most Python code in general. Maybe the issue with inconsistent data that users have to check is something that needs to be addressed by Ansible?

Well, since we need this as part of generating documentation, I don't think this really is an Ansible-specific issue. And I have proposed this for Ansible specifically, but IMO proper type-checking for all types is something the language could help with.

I understand that if you are using Jinja for web-based templating and you have control over the data, this may be less of an issue, but Jinja is not (no longer) used as a one-purpose template engine.

And yes, we are using constructions like {% if value is sequence and value is not string %}, which I'd rather not use if I didn't have to.

@davidism So despite the naming of non-string sequences, you object to everything else too ? Testing for booleans is a dangerous thing too if there's no 'boolean' test. Without some of these tests Jinja is a minefield to most non-experts.

@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Nov 20, 2018

Some real-life advice on forums:

If you don't think there's a problem in Jinja (beyond your use-cases), StackOverflow is a good reality check IMO. And I had some of the same issues before I decided we could do better and created the PR in Ansible and upstreamed it here.

@dagwieers dagwieers force-pushed the dagwieers:new-type-tests branch from ba18cdb to a442d4d Nov 20, 2018
@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 20, 2018

I'm not saying your use case isn't important, I'm trying to understand where this belongs and why users are trying to do these things in the first place.

And if we add these, why not every type in builtins and every abc in collections? And what about user-provided types? Maybe there's a better solution to checking types instead of adding individual tests for each?

@flexferrum

This comment has been minimized.

Copy link
Contributor

flexferrum commented Nov 20, 2018

if a value is an int or a float in the context of a template

I suppose, arithmetic operations on ints and floats will have different results. Formatting of big numbers as well.

And if we add these, why not every type in builtins and every abc in collections? And what about user-provided types? Maybe there's a better solution to checking types instead of adding individual tests for each?

May be extra 'universal' tester could help? Something like: true is typeof(boolean) or true is typeof(name='boolean') ?

@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Nov 20, 2018

In our case the documentation is generated from contributed YAML files, so we do want to ensure that we handle user-contributed input correctly and produce correct output regardless of the input type.

Here is one of the use-cases where we added the list-test ourselves: https://github.com/ansible/ansible/pull/37514/files

So we have cases where the data provided could be a string or a list, and depending on which we also have to check if a value is the same as the string, or is included in the lists.

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Nov 21, 2018

I'm still interested in exploring the last question I asked:

Maybe there's a better solution to checking types instead of adding individual tests for each:

If we merge this as-is, rather than exploring that, we still need to decide on a name for the list test. I'm thinking it might be easier to add a string=False parameter to the sequence test rather than using imprecise or too-long names. Might make sense to deprecate the current behavior and transition to that as the default, since I think it's what people usually intend with sequence.

We should also consider @abadger's point about whether we want to test iterable vs sequence.

@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Nov 21, 2018

If we would go to a hand-off type of test, I'd rather have a 'type' test in the same light as 'sameas', rather than having switches to existing tests. (typeof doesn't sound right, oftype doesn't look right...)

{% if variable is type boolean %}
{% if variable is type integer %}
{% if variable is type float %}
{% if variable is type string %}
{% if variable is type list %}
{% if variable is type mapping %}

Should we be testing for python types, or rather switch to Jinja naming above ?
And this does not bring us any closer to naming the tests for non-string sequences :-/

@giovanniborella

This comment was marked as off-topic.

Copy link

giovanniborella commented Jan 16, 2019

+1 for this feature

@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Jan 16, 2019

Can someone make a (design) decision on this topic?
So that we can move forward with an implementation.

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Jan 16, 2019

I'm fine with it except for the list test, which I'm not completely satisfied with since it doesn't describe the actual test and might cause issues if someone actually wants to test for lists. I haven't done anything with it because I'm currently working on Werkzeug and Flask. Plus one and bump comments just get minimized or deleted though, they're noise.

@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Jan 16, 2019

I will remove the list type checking from this PR as to not hijack everything else.

@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Jan 16, 2019

Please review!

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Jan 16, 2019

I mean, I think such a test makes sense, I just want a good name. It's not hijacking the PR, since I'm not planning to merge it right this moment anyway. But we can leave it for another time, that's fine too.

Copy link
Member

davidism left a comment

Needs a changelog entry as well.

tests/test_tests.py Outdated Show resolved Hide resolved
tests/test_tests.py Outdated Show resolved Hide resolved
@dagwieers dagwieers force-pushed the dagwieers:new-type-tests branch from c9996c3 to 17a9cf4 Jan 16, 2019
@dagwieers

This comment has been minimized.

Copy link
Contributor Author

dagwieers commented Jan 16, 2019

I updated the tests, add a changelog entry and updated the docs.

This PR adds a few more type-related tests.

- boolean
    Testing of an object is a boolean required 2 tests.

- false
    Make this similar to testing none value

- true
    Make this similar to testing none value

- integer
    The existing 'number' test does not make a distinction between
    integer, float or even booleans

- float
    The existing 'number' test does not make a distinction between
    integer, float or even booleans
@davidism davidism force-pushed the dagwieers:new-type-tests branch from 1945f22 to 9bd3cb2 Oct 10, 2019
@davidism davidism added this to the 2.11.0 milestone Oct 10, 2019
@davidism davidism merged commit 0b9d252 into pallets:master Oct 10, 2019
10 checks passed
10 checks passed
Tests Build #20191010.3 succeeded
Details
Tests (Job Docs) Job Docs succeeded
Details
Tests (Job PyPy 3 Linux) Job PyPy 3 Linux succeeded
Details
Tests (Job Python 2.7 Linux) Job Python 2.7 Linux succeeded
Details
Tests (Job Python 2.7 Windows) Job Python 2.7 Windows succeeded
Details
Tests (Job Python 3.5 Linux) Job Python 3.5 Linux succeeded
Details
Tests (Job Python 3.6 Linux) Job Python 3.6 Linux succeeded
Details
Tests (Job Python 3.7 Linux) Job Python 3.7 Linux succeeded
Details
Tests (Job Python 3.7 Mac) Job Python 3.7 Mac succeeded
Details
Tests (Job Python 3.7 Windows) Job Python 3.7 Windows succeeded
Details
@davidism

This comment has been minimized.

Copy link
Member

davidism commented Oct 10, 2019

Thanks for working on this, it's marked for 2.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.