Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#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 Aymeric Augustin, 10 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Adam Zapletal, 10 years ago

Owner: changed from nobody to Adam Zapletal
Status: newassigned

comment:3 by Adam Zapletal, 10 years ago

Has patch: set

comment:4 by Preston Timmons, 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 Preston Timmons, 10 years ago

Resolution: invalid
Status: assignedclosed

comment:6 by Adam Zapletal, 10 years ago

No worries! I'm glad my patch helped with the closing of this ticket somehow ;)

Note: See TracTickets for help on using tickets.
Back to Top