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

Actually use the Jinja2 template backend #1882

Merged
merged 2 commits into from
Jul 5, 2024
Merged

Actually use the Jinja2 template backend #1882

merged 2 commits into from
Jul 5, 2024

Conversation

matthiask
Copy link
Member

Refs #1881.

Description

Please include a summary of the change and which issue is fixed. Please also
include relevant motivation and context. Your commit message should include
this information as well.

Fixes # (issue)

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@tim-schilling
Copy link
Member

I pushed this forward for the next person. Here's my message from the last commit:

It instruments the single template render, but not the inherited
templates and I'm guessing not the included templates either.
I suspect we're going to have to patch jinja templates more robustly
than relying on the django jinja backend template class.

@matthiask
Copy link
Member Author

@tim-schilling It would be great if we were able to inspect all of Jinja2's rendering but maybe that's harder because of Jinja2's template compilation, or what do you think? I didn't do much research here, so I'm basically just speculating.

I think what we have with your changes is already much better than the status quo. We could merge this, document the limitations and move on?

@AlexCLeduc
Copy link
Contributor

For what it's worth, this patch on its own would be very useful for my jinja projects. I very rarely use the toolbar to inspect parents/included templates.

@matthiask
Copy link
Member Author

@tim-schilling Do you have any reservations against merging this as it is, since it's more useful than the status quo?

@tim-schilling
Copy link
Member

No, it is better. I'd prefer to document the caveat better for sure, but if it helps others that's still a win.

@tim-schilling
Copy link
Member

@matthiask this is ready for your review. I don't think you'll be able to give it a green checkmark since you opened the PR.

@tim-schilling
Copy link
Member

I'm going to assume your previous comments were tacit approval. I'm about to head out for the day and I'd like to get this in for 4.4.4 with #1946

matthiask and others added 2 commits July 5, 2024 06:15
- Add jinja2 template to example app.
- Switch to the render function to include context.

It instruments the single template render, but not the inherited
templates and I'm guessing not the included templates either.
I suspect we're going to have to patch jinja templates more robustly
than relying on the django jinja backend template class.

Co-Authored-By: Tim Schilling <[email protected]>
@tim-schilling tim-schilling merged commit 4fd886b into main Jul 5, 2024
46 of 49 checks passed
@matthiask
Copy link
Member Author

Thank you! I'm late to the party today.

@matthiask matthiask deleted the jinja2 branch July 5, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants