Ignore --fail-on-template-vars with default filter?

[blueyed]

django/django#8200 was not accepted, but I think it's good to not fail with {{ foo | default:"bar" }} with foo being undefined.

@codingjoe
What do you think?

[codingjoe]

I don't know. It's currently failing whenever Django raises a VariableDoesNotExist error. I don't know if it makes sense to add exception to that.

[blueyed]

The point of the default filter is to handle this, isn't it?

[codingjoe]

Hm... default is for false values, not for none existing ones. So if your value foo is False it will not complain. If it does not exist it will.

[blueyed]

Fair point.
For reference, there is also default_if_none: https://docs.djangoproject.com/en/1.10/ref/templates/builtins/#default-if-none

[jcushman]

I think this is a good feature request. It's a valid and common pattern to have in base.html {{ foo|default:"Sitewide Foo" }} and then for individual pages to provide 'foo': 'Page Specific Foo' to the context if they want to override. It's a false positive when --fail-on-template-vars flags that as an error.

I don't think it's accurate that "default is for false values, not for none existing ones". Django documentation specifies that non-existent values render to empty string (and that the built-in templates rely on this), and that {{ ''|default:'foo' }} renders to foo. The behavior of non-existent values with the default filter is documented and supported.

[codingjoe]

@jcushman you make a good point, but one could argue the other way, that a context should be consistent, no matter what scenario. Values can be None, but the key should exist.
Both points would make sense to me. With that in mind, I don't know if the feature requested here is implementable, without doing something nasty or driving code complexity.
I am currently not sure, if the results will justify the means, but I also haven't seen an approach to implement this yet.
If you have a suggestion on how this could be implemented, go for it!

[bluetech]

@jcushman Thanks for PR #922, I'm replying in the issue to keep the discussion in one place.

I don't think it's accurate that "default is for false values, not for none existing ones".

I think as long as default does not explicitly handle non-existing variables, then it is effectively accurate. @blueyed tried to change Django but it was rejected, although I think if someone opens an issue for discussion and proposes a clean implementation, then it might be accepted there. Until then, I don't think pytest-django diverge from Django on that.

BTW, from the Django docs:

This behavior is slightly different for the if, for and regroup template tags. If an invalid variable is provided to one of these template tags, the variable will be interpreted as None. Filters are always applied to invalid variables within these template tags.

I haven't tried it, but this implies that you can replace { foo|default:'Foo' } with {% if foo is not None %}{ foo }{% else %}Foo{% endif %}, without triggering the failure. This is certainly not as nice, but might be a valid workaround for the few cases which expect non-existing variables, as in the django-pipeline case.

[codingjoe]

I looked into the documentation for both the default and default_if_none template tags. Both note only False and None values, but are not meant to mitigate missing context variables. Using an if statement to wrap missing template variables seems to be the intended way to handle this in Django.

I understand the frustration, but this warning is meant to keep inform if you have diverted from the intended path. Therefore, I must conclude that the feature is correct right now, and I am not in favor of #922. I hope you can understand.

[jcushman]

I think as long as default does not explicitly handle non-existing variables, then it is effectively accurate.

I looked into the documentation for both the default and default_if_none template tags. Both note only False and None values

Sorry to go on about this so much, but this really feels like a misunderstanding.

default doesn't look for False, it looks for Falsey -- variables that "evaluate to False", explicitly including the empty string. (docs link)

Missing variables are explicitly defined to resolve to the empty string, unless a test harness like pytest-django is temporarily setting it to something else only for testing. (docs link)

If missing_variable is explicitly defined to be '', which it is, and ''|default:'Foo' is explicitly defined to be 'Foo', which it is, then missing_variable|default:'Foo' is explicitly defined to be 'Foo'.

And there's a Django test that connects these dots, making sure that {{ var|default:"Foo" }} is "Foo" for a missing value as long as string_if_invalid hasn't been modified.

This behavior is documented and depended on elsewhere in Django. For example the docs for {% include %} show this example:

# The name_snippet.html template:
    {{ greeting }}, {{ person|default:"friend" }}!
# The including template:
    {% include "name_snippet.html" with greeting="Hi" only %}

Should we send a pull request asking Django to change that documentation because pytest-django flags it as bad style and recommends the longer version {% if person %}{{ person }}{% else %}friend{% endif %}? Why would that pull request be accepted? The current example works correctly and is documented and tested to work correctly.

This is certainly not as nice, but might be a valid workaround for the few cases which expect non-existing variables, as in the django-pipeline case.

Django works this way because it's nicer in common cases, not just rare cases. A very common case is template inheritance:

# base.html
<head>
  <meta name="description" content="{{ description|default:"sitewide default description" }}">
</head>

# most views use the default description
def my_view(request):
    return render("my_page.html")

# minority of views that want to override description
def my_special_view(request):
    return render("my_special_page.html", {'description': 'my special description'})

I think you're saying that this example code is bad style and diverges from Django's documentation, and the correct way would be the longer alternative {% if description }}{{ description }}{% else %}{{ sitewide default description }}{% endif %}. But where does that idea of bad style come from? This example looks like something out of a Django tutorial, not some weird edge case. It's documented to work and it's specifically supported in Django's test suite. Why would pytest-django enforce a need to use a longer form in this situation?

That's where the problem with third party packages like django-pipeline comes in. For myself, I can adhere to pytest-django's preferred linting style in exchange for getting warnings for {{ description }} that I actually want, even if it seems arbitrary to me. But if I start sending pull requests to third party packages that use the shorter form, why would they accept them, when the pull request makes their template harder to read and doesn't make it more correct?

[bluetech]

And there's a Django test that connects these dots, making sure that {{ var|default:"Foo" }} is "Foo" for a missing value as long as string_if_invalid hasn't been modified.

I think this test actually demonstrates the opposite -- that when string_if_invalid is set, then default shouldn't work. If string_if_invalid is meant for testing purposes, this shows that Django thinks that such cases should be flagged.

I understand where you're coming from, but this seems like something that should be resolved on the Django side. Either they say:

• default handles invalid -> string_if_invalid should not affect it (the behavior and the test should change).

or

• default doesn't handle invalid -> templates which want to be "invalid-robust" should use some else that handles it, like if.

For myself, I think it would have been better if Django made default handle invalid, but it doesn't. It doesn't seem right to me for pytest-django to do something which Django itself rejected.

[jcushman]

If string_if_invalid is meant for testing purposes, this shows that Django thinks that such cases should be flagged.

Here's what the docs say:

While string_if_invalid can be a useful debugging tool, it is a bad idea to turn it on as a ‘development default’.
Many templates, including some of Django’s, rely upon the silence of the template system when a nonexistent variable is encountered. If you assign a value other than '' to string_if_invalid, you will experience rendering problems with these templates and sites.
Generally, string_if_invalid should only be enabled in order to debug a specific template problem, then cleared once debugging is complete.

default handling invalid is supported and documented, such as in the {% import %} example. It's changing string_if_invalid that isn't supported.

[jcushman]

(And, it's really cool that pytest-django is able to get something useful out of string_if_invalid, even though it's not supposed to be used this way! It's just that because string_if_invalid isn't supposed to be used this way, pytest-django needs some extra inspections to conform to Django's behavior when string_if_invalid isn't set.)

[codingjoe]

Hi there,

So, there was no misunderstanding on my side, I simply have a different opinion. Don't get me wrong, I understand your reasoning, but I am simply using it differently right now, thus have limited intention of changing that. I believe it's clear at this point, that you are proposing a breaking change unless it would be an optional change, that people would need to activate.

And this is really where I have a problem. I don't want to bloat this package too much. It's hard to maintain packages and adding more features makes it harder. Especially if you base your feature on undocumented APIs that may break in any release.

I wrote the original implementation, and I am happy to maintain its functionality going forward. However, I am very hesitant to blow it up too much.

Allow me one last side note. You correctly mentioned falsely values. However, a VariableDoesNotExist is a wrapper for either an IndexError, ValueError, KeyError or TypeError. Swallowing that with a simple default filter is in my experience dangerous. Because it doesn't need to be missing in the context, there can be a host of reason, why any of these errors is raised.

Best,
Joe

[jcushman]

I don't want to bloat this package too much.

Fair enough! You asked before to see a suggestion for how to implement it, so the PR was meant to show how complex that would be. If it's too complex it's too complex. 🤷‍♂️

Swallowing that with a simple default filter is in my experience dangerous. Because it doesn't need to be missing in the context, there can be a host of reason, why any of these errors is raised.

For sure, but exactly the same errors will be swallowed by {% if foo.bar %}{{ foo.bar }}{% else %}baz{% endif %} as by {{ foo.bar|default:"baz" }}, right? Django does slightly different things under the hood but each is as dangerous as the other. If you flag the short one but not the long one you're just nudging people to be a little more verbose in their templates, which hasn't reduced errors in my experience since the logic is harder to read.

In a perfect world it would be great to have two separate lints like

• --fail-on-unhandled-template-vars that allowed both of those patterns, but not bare {{ missing }} (this is what I'd use)
• --fail-on-all-template-vars that blocked both of them (this is what I'd suggest for people who want to know about all those forms of error you mentioned)

But I definitely get that "fail when Django calls string_if_invalid" is the easiest thing to maintain!