Skip to content

allow optional use of a filter based on its existence#1248

Merged
davidism merged 1 commit intopallets:masterfrom
MLH-Fellowship:test-for-filter
Apr 4, 2021
Merged

allow optional use of a filter based on its existence#1248
davidism merged 1 commit intopallets:masterfrom
MLH-Fellowship:test-for-filter

Conversation

@amy-lei
Copy link
Copy Markdown
Contributor

@amy-lei amy-lei commented Jun 26, 2020

Resolves #842

  • Undefined filters nested in if/elif/else statements and conditional expressions will cause an error only if executed during runtime

Files changed:

  • nodes.py
  • compiler.py
  • runtime.py

Comment thread src/jinja2/runtime.py Outdated
@davidism davidism added this to the 3.0.0 milestone Mar 27, 2021
@amy-lei amy-lei changed the title #842: Optionally use a filter based on its existence allow optional use of a filter based on its existence Mar 30, 2021
Comment thread src/jinja2/compiler.py Outdated
@amy-lei amy-lei force-pushed the test-for-filter branch 3 times, most recently from c0fb589 to 541bf72 Compare April 1, 2021 23:29
@ThiefMaster
Copy link
Copy Markdown
Member

Are we covering cases like {{ 'foo'|nosuchfilter if false else 'bar' }} and {{ 'foo' if true else 'bar'|nosuchfilter }} - neither should raise (since the filter is not actually used, but based on the code it looks like it may raise..

@amy-lei
Copy link
Copy Markdown
Contributor Author

amy-lei commented Apr 2, 2021

Are we covering cases like {{ 'foo'|nosuchfilter if false else 'bar' }} and {{ 'foo' if true else 'bar'|nosuchfilter }} - neither should raise (since the filter is not actually used, but based on the code it looks like it may raise..

Right, I forgot to account for those but probably should (and will) too.

@ThiefMaster
Copy link
Copy Markdown
Member

I have the feeling any block-level check is likely to have problematic edge cases - can't we just let it fail when actually accessing the filter?

@amy-lei
Copy link
Copy Markdown
Contributor Author

amy-lei commented Apr 2, 2021

I have the feeling any block-level check is likely to have problematic edge cases - can't we just let it fail when actually accessing the filter?

Hmm that's true. The error won't be as helpful though: TypeError: 'NoneType' object is not callable

@ThiefMaster
Copy link
Copy Markdown
Member

Good point, that error is awful and filters that are sometimes but not always undefined are very much an edge case for which we probably shouldn't sacrifice error verboseness with other, more common, cases

@ThiefMaster
Copy link
Copy Markdown
Member

Could we compile filters to something like this at the beginning of the template?

try:
    filter_tmp_0 = self.filters['filter_name']
except KeyError:
    @internalcode
    def filter_tmp_0(*unused):
        raise TemplateRuntimeError("no filter named 'filter_name'")

Then the code where the filter is actually used just uses filter_tmp_0 which will either call the filter or raise an exception - without adding extra runtime overhead every time the filter is actually invoked.

@amy-lei
Copy link
Copy Markdown
Contributor Author

amy-lei commented Apr 2, 2021

Ooh that's a good idea! I moved it to pull_dependencies in the new commit. Let me know if I misunderstood. Will address conditional expressions next.

@davidism
Copy link
Copy Markdown
Member

davidism commented Apr 4, 2021

I'm working on a separate PR to create an if 'name' is filter test.

@davidism davidism merged commit af5d80e into pallets:master Apr 4, 2021
@davidism davidism deleted the test-for-filter branch April 4, 2021 17:20
@davidism davidism mentioned this pull request Apr 5, 2021
6 tasks
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

optionally use a filter based on its existence

3 participants