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)

by robert@…, 18 years ago

Attachment: django-include-tag.diff added

The patch

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

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 by rjwittams, 18 years ago

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 by L.Plant.98@…, 18 years ago

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 by rjwittams, 18 years ago

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

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

Patch updated for subversion rev 880 plus

by rjwittams, 18 years ago

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

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

comment:5 by hugo, 18 years ago

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.

by rjwittams, 18 years ago

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

Updated to fix hugos bug

by jhf@…, 18 years ago

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

Fix minor bugs to work properly with revision 1080

comment:6 by Adrian Holovaty, 18 years ago

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