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

Adds ability to pass local context into included templates. #512

Closed
wants to merge 1 commit into from

Conversation

legutierr
Copy link

This enhancement implements functionality discussed in #241. Tests are included.

@mitsuhiko
Copy link
Contributor

I really hate the include tag and wish I would never have added it. Because of that I'm not so sure if I want to extend it at this point. It's so buggy :(

@legutierr
Copy link
Author

Hi Armin,

I'm really surprised to hear that you regret adding the include tag! It has been invaluable for us; I can't imagine living without it.

We have found that for designers and CMS users the learning curve for writing macros is quite a bit higher than for writing simple templates. While there are some advantages to importing macros instead of including templates, in practical situations for us, we have found that for those key users the advantages are outweighed by the additional complexity. We have also found that composition, re-use and testing are simplified when UI snippets can be written as standalone templates rather than macros that need to be imported and then invoked.

I have a feeling that our experience in this regard is not uncommon. I'm confident that the fact that you did add the include tag to jinja is very much appreciated by a large number of people :)

This pull-request includes code that we have been using to provide some greater control over what context is accessible within included templates. In our use case, it allows us to give CMS users access to precisely the data they need for their snippet, and nothing more. We got the idea from Twig, but as you see, the concept (with a slightly different implementation) is now found in Django as well.

I'm happy to add documentation for this feature, if you are willing to move forward. I didn't do that here because I wasn't sure what your preferences were for documentation.

@hamishtaplin
Copy link

I'd love to see this merged, coming from Twig it would make Jinja so much easier to work with as the current solution of just declaring variables with set outside the include is super clunky.

@davidism
Copy link
Member

Thanks for working on this. While the implementation seems fairly straightforward, it doesn't match the style used in other tags such as {% trans %} and {% with %}. As was pointed out, with and set already supports the majority of this functionality, at the cost of a bit more typing. Many of the remaining open issues deal with scope, and I think there's value in evaluating and refactoring Jinja's scoping code at some point, at which point this might be considered again. Closing for now.

@davidism davidism closed this Jan 11, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
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.

4 participants