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

Fix #2109: Recursively unwrap loaders to support template partials #2117

Merged
merged 6 commits into from
Apr 2, 2025

Conversation

matthiask
Copy link
Member

@matthiask matthiask commented Mar 31, 2025

Description

The current template source view only supports unwrapping one level of
template loaders depending on other loaders.

When using django-template-partials with cached templates we have to
unwrap the loader twice to get to the concrete loaders.

Fixes #2109

Checklist:

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

@matthiask matthiask force-pushed the 2109-loader-unwrapping branch from a6a984f to f7bb5a4 Compare March 31, 2025 21:34
@matthiask
Copy link
Member Author

Note! The first commit adds the passing test, the second commit adds
the template partials integration (which causes the test to fail) and the
third commit fixes the issue. The change can be squashed, this is just a note for reviewers.

@matthiask matthiask requested a review from tim-schilling April 1, 2025 08:57
# We are not actively using template-partials; we just want more nesting
# in our template loader configuration, see
# https://github.com/django-commons/django-debug-toolbar/issues/2109
"template_partials",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we always be testing with template partials? I think with the django-csp tests, we have specific tests that install the package and test against it. That said, this is likely to be a short-time thing. Both django-csp and template-partials are very likely to be merged to core in the 6.x series.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit bad, I agree. Another way to test this would have been to add a mock loader with a loaders attribute containing the cached loader configuration and testing the panel like that. I thought it easier and more understandable to add django-template-partials as a test dependency than doing the more involved mocking.

We will have to revisit this when template-partials is merged (hopefully), either by switching to a different package which also wraps the cached loader or by adding the relevant testing code to our project.

Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this issue! My other comment isn't a required change, but a discussion topic for us.

@matthiask matthiask merged commit 1ea7c95 into django-commons:main Apr 2, 2025
25 checks passed
@matthiask matthiask deleted the 2109-loader-unwrapping branch April 2, 2025 11:28
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.

django-template-partials and the templates panel do not work well together it seems
2 participants