#24374 closed Cleanup/optimization (invalid)
IncludeNode should use Template._render rather than Template.render
Reported by: | Preston Timmons | Owned by: | Adam Zapletal |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When Template.render is called, it creates a new stack on the context. It also potentially sets context.template and adds the context processors to RequestContext.
I'm pretty sure this is unnecessary since the IncludeNode already pushes to the context stack or creates a new context internally. Calling Template._render here would be a slight optimization and remove the need for the conditional logic to set context.template on toplevel_render.
All the tests seem to pass if this change is made.
Change History (6)
comment:1 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 10 years ago
Has patch: | set |
---|
comment:4 by , 10 years ago
Hi adamzap. I hope this isn't discouraging, but I'm going to close this ticket as invalid. There are subtle differences between context.push()
and context.render_context.push()
that I didn't understand when I opened this ticket. The former happens in the IncludeNode
, but the latter as part of Template.render()
. Although the tests don't catch it, changing to Template._render()
here would introduce a bug for IncludeNode
similar to #24555.
Anyhow, thanks for making a PR, but sorry I forgot to close this one earlier.
comment:5 by , 10 years ago
Resolution: | → invalid |
---|---|
Status: | assigned → closed |
comment:6 by , 10 years ago
No worries! I'm glad my patch helped with the closing of this ticket somehow ;)
Pull request is here: https://github.com/django/django/pull/4479