Opened 18 years ago

Closed 18 years ago

Last modified 17 years ago

#598 closed enhancement (fixed)

[patch] Include tag

Reported by: robert@… Owned by: Adrian Holovaty
Component: contrib.admin Version:
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is the include tag I am using in the new-admin branch and in my own projects.
The reasons why {% include "path/to/template" %} is better / more useful than {%ssi /absolute/path/to/template parsed %}:

ssi is a wierd name that only makes sense if you know the history of httpds.

"Server side? This whole thing is server side, right?"

Absolute paths are useless for redistribution, do not allow overriding in different template paths.

"parsed" is a bit arbitrary and meaningless to the target audience for templates.

In {% include "path" %}, constant strings get special cased and hoisted up into the calling template instance rather than being looked up every time. Variables can also be used.

Attachments (5)

django-include-tag.diff (3.1 KB) - added by robert@… 18 years ago.
The patch
django-include-tag-svn880.diff (2.4 KB) - added by L.Plant.98@… 18 years ago.
Patch updated for subversion rev 880 plus
django-include-tag-2.diff (2.1 KB) - added by rjwittams 18 years ago.
updated patch - moved tag to template/loaders.py along with other loader tags.
django-include-tag-3.diff (2.1 KB) - added by rjwittams 18 years ago.
Updated to fix hugos bug
django-include-tag-4.diff (2.0 KB) - added by jhf@… 18 years ago.
Fix minor bugs to work properly with revision 1080

Download all attachments as: .zip

Change History (11)

Changed 18 years ago by robert@…

Attachment: django-include-tag.diff added

The patch

comment:1 Changed 18 years ago by L.Plant.98@…

Questions about the patch:
1) what is the point of this line in your current patch?:

646: parsed = False

2) It looks like the behaviour depends on whether the path argument is quoted or not. If that's right, is it really a good idea? and shouldn't it be documented? The two are modes are sufficiently different to warrant either a parameter or different tags -- maybe 'insert' or 'readfile' for static files, 'include' for templates.

Cheers,

Luke

comment:2 Changed 18 years ago by rjwittams

yep, that assignment on line 646 is pointless.

The constant include optimisation doesn't actually change behaviour, just makes it slightly more efficient.

The point is that this takes a variable as its first argument. If that variable happens to be a constant string,
it is loaded only once rather than being loaded every time it is encountered ( ie in a loop).

ie {% include path_var %} is slightly less efficient than {% include 'path/to/template' %}

So it would kind of remove the point of the optimisation to split this into two tags. To be honest, I would be in support of getting rid of all non-quoted free-form arguments. They are confusing.

And this doesn't deal specially with static files either. Everything is treated as a template. I agree that that might be
worth another tag, but I've not come across a need for it. So files with no tags or variables will work, but they will be one big TextNode and a bit of time will be wasted parsing them.

comment:3 Changed 18 years ago by L.Plant.98@…

OK, ignore my second point, I didn't read the patch very carefully (or did so too late in the day!). I think there does need to be some more consistency about arguments and quoting in the template tags.

comment:4 Changed 18 years ago by rjwittams

No worries, it needed more explanation. Cheers for catching the pointless assignment.

Changed 18 years ago by L.Plant.98@…

Patch updated for subversion rev 880 plus

Changed 18 years ago by rjwittams

Attachment: django-include-tag-2.diff added

updated patch - moved tag to template/loaders.py along with other loader tags.

comment:5 Changed 18 years ago by hugo

When playing with this tag, I sometimes got weird errors when using constant names for the included template about NoneType not having get_node_by_type or something like that. It was easily fixed by changing the ConstantIncludeNode from:

class ConstantIncludeNode(Node): 
    def __init__(self, template_path): 
        try: 
            t = get_template(template_path) 
            self.nodelist = t.nodelist 
        except Exception, e: 
            self.nodelist = None

    def render(self, context): 
        if self.nodelist: 
            return self.nodelist.render(context) 
        else: 
            return '' 

to:

class ConstantIncludeNode(Node): 
    def __init__(self, template_path): 
        try: 
            t = get_template(template_path) 
            self.template = t
        except Exception, e: 
            self.template = None 

    def render(self, context): 
        if self.template: 
            return self.template.render(context) 
        else: 
            return '' 

But I am all +1 for applying something like this, because it's much more usefull than ssi.

Changed 18 years ago by rjwittams

Attachment: django-include-tag-3.diff added

Updated to fix hugos bug

Changed 18 years ago by jhf@…

Attachment: django-include-tag-4.diff added

Fix minor bugs to work properly with revision 1080

comment:6 Changed 18 years ago by Adrian Holovaty

Resolution: fixed
Status: newclosed

(In [1349]) Fixed #598 -- Added {% include %} template tag. Added docs and unit tests. Thanks, rjwittams

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