Opened 19 years ago
Closed 14 years ago
#1105 closed defect (duplicate)
[patch] simple_tag decorator enhancement
Reported by: | Owned by: | Julien Phalip | |
---|---|---|---|
Component: | Template system | Version: | |
Severity: | normal | Keywords: | |
Cc: | django@…, semenov@… | Triage Stage: | Design decision needed |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Attached is a patch for a tag function decorator that I find useful. It's a simple extension of Robert's simple_tag. The tag function is passed the context as the first argument and also any arguments to the tag, resolved in the same way as for the simple_tag decorator. The decorated function can manipulate the context and either return a string, which will be inserted into the template, or None.
For example, this tag puts the project settings into the context:
@register.simple_tag_with_context def get_settings(context): """ Put the project settings into the context. Usage:: {% get_settings %} """ from django.conf import settings context['settings'] = settings
And this one is the equivalent of the existing simple_tag example in NewAdminChanges:
@register.simple_tag_with_context def monkey_tag(context, verb): return "I %s no evil" % verb
Attachments (7)
Change History (30)
by , 19 years ago
Attachment: | simple_tag_with_context_r1764.diff added |
---|
comment:1 by , 19 years ago
I would prefer this to be similar to the inclusion_tag , ie optionally take an argument of takes_context=1. This is slightly tricky due to how decorators work ( making the no args and args versions work the same). register_tag does this correctly.
Going further, I'd like to split this stuff out a bit more. gen_compile_func should make use of a function attribute, eg arg_info. This would be a list of stuff that could be taken into account when creating the compile function.
Decorators should then be provided, eg
@register.simple_tag
@takes_context
@takes_block
def my_tag(context, block, arg1, arg2):
pass
This way the concerns are separated a bit.
comment:2 by , 19 years ago
Summary: | [patch] simple_tag_with_context decorator → [patch] simple_tag decorator enhancement |
---|
I've reimplemented simple_tag in line with Robert's first suggestion.
Now simple_tag passes in context and/or block arguments (always in that order)
if requested. If a block is requested then the tag will automatically consume
the template from {% tagname %} until {% endtagname %} passing the resulting
nodelist in as the block argument.
The comment
tag can now be implemented:
@register.simple_tag(takes_block=True) def comment(block): pass
And the upper
tag example from the template documentation becomes:
@register.simple_tag(takes_context=True, takes_block=True) def upper(context, block): return block.render(context).upper()
Robert, I'm not sure if I like the idea of manipulating simple_tag with accessory
decorators. As far as I can tell what you are proposing is just a synatactic hack to pass
arguments to the simple_tag decorator; the accessory decorators would have no obvious meaning except in the context of simple_tag. I think I prefer the syntax above because there really is only one decorator being applied to the function (but if the original debate about Python decorator syntax is anything to go by then the "right" way will have to be determined by a benevolent dictator;).
comment:3 by , 19 years ago
Type: | enhancement |
---|
comment:4 by , 18 years ago
Owner: | changed from | to
---|---|
Type: | → defect |
comment:5 by , 18 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
Seems like a useful enhancement.
comment:7 by , 16 years ago
Cc: | added |
---|---|
Component: | Core framework → Template system |
Owner: | changed from | to
I have made a patch (super_simple_tag.diff) that improves simple_tag quite a bit. It can now take the context and also the inner block.
It is backward compatible, so you still can do:
@register.simple_tag def my_tag(bla):
Now you can also do:
@register.simple_tag(takes_context=True) def my_tag(context, bla):
If you're making a block tage, e.g:
{% mytag "blablabla" %} .... {% endmytag %}
then you can also do:
@register.simple_tag(takes_block=True) def my_tag(block_nodelist, bla):
or even:
@register.simple_tag(takes_block=True, takes_context=True) def my_tag(block_nodelist, context, bla):
follow-up: 9 comment:8 by , 16 years ago
Now, the patch seems to work pretty well. I just came across an oddity with block tags. When you do block_nodelist.render(context) it does not render the children templates content properly. You can reproduce that with something like this:
@register.simple_tag(takes_block=True, takes_context=True) def customspaceless(context, block_nodelist): from django.utils.html import strip_spaces_between_tags return strip_spaces_between_tags(block_nodelist.render(context).strip())
In parent.html:
{% customspaceless %} header {% block content %} {% endblock %} footer {% endcustomspaceless %}
In child.html:
{% extends "parent.html" %} {% block content %} middle {% endblock %}
That will render:
headerfooter
(the child's 'middle' is left out)
Can you spot what's the problem here?
My first assumption is that the nodelist is generated at the right time. Currently I generate it in the generic_tag_compiler function:
def generic_tag_compiler(params, defaults, name, node_class, parser, token, takes_context=False, takes_block=False): ... if takes_block: nodelist = parser.parse(('end%s' % name,)) parser.delete_first_token() node_class = curry(node_class, block_nodelist=nodelist)
Maybe that should be done elsewhere. Any idea?
comment:9 by , 16 years ago
Replying to julien:
Block substitution depends on Node.get_nodes_by_type()
, which will look for a nodelist
attribute. So probably you just have to rename block_nodelist
to nodelist
to fix this.
comment:10 by , 16 years ago
Thanks emulbreh, well spotted!
I have updated the patch, which is now fully fonctional. Will post documentation and test shortly.
comment:11 by , 16 years ago
line 913 of init.py looks off:
if takes_context != None or takes_context != None:
I think maybe the second takes_context should be takes_block
comment:12 by , 16 years ago
Quick style note, when comparing for none you should generally do if var is None
or if var is not None
, instead of using the equality or inequality operator.
by , 16 years ago
Attachment: | super_simple_tag.diff added |
---|
Patch + tests + doc (created from revision 7774)
comment:13 by , 16 years ago
Thanks Alex and justinf, I've applied your suggestions in the new patch.
by , 16 years ago
Attachment: | 1105.simple_tag.r7813.diff added |
---|
takes_nodelist instead of takes_block
comment:14 by , 16 years ago
I'd prefer a takes_nodelist
argument since you will get a NodeList
that has nothing to do with {% block %}
.
by , 16 years ago
Attachment: | improved_simple_tag.r7818.diff added |
---|
Replaced "takes_block" by "takes_nodelist", and "block_nodelist" by "nodelist". Amended the doc and tests as well.
comment:15 by , 16 years ago
Thanks emulbreh for the suggestion. I too prefer takes_nodelist.
I made the change in the latest patch. I kept my previous patch as a base as it takes care of the doc and tests. It is also necessary not to set the "nodelist" attribute if it is None (see SimpleTag.init).
comment:17 by , 16 years ago
Triage Stage: | Accepted → Design decision needed |
---|
Marking DDN as per Malcolm's email on the dev-list:
http://groups.google.com/group/django-developers/msg/7692178f04969f5b
Specifically responding to Malcolm's comment:
[...] we need to consider whether adding something like "takes_nodelist" is really a good idea for something called *simple* tag decorator.
I think that it is worth having takes_nodelist
, as it does not necessarily "complexify" simple_tag
. My interpretation of "simple tag", in this context, is two-fold: "simple to create a template tag" and "tag with simple syntax". The proposed patch preserves both of those.
Conversely, a complex tag would be something with a complex syntax (e.g. {% if ... and not ... %}
, or {% get_comment_count for entry as comment_count %}
), and therefore would justify getting your hands dirty with variable resolving and tokens.
At this stage, there is no way to "simply" create a "simple" block tag.
comment:18 by , 16 years ago
I was wondering: if support for the "takes_context" addition to "simple_tag" is uncontentious could a fresh request be spawned for it so that the need for a decision about the extra "takes_nodelist" stuff doesn't further hold up or prevent that support for "takes_context" being added to simple_tag. If spawning a fresh ticket is not a sensible/desirable thing to do (esp. if I've misunderstood and takes_context support actually is contentious too), fair enough, but thought I'd ask :)
comment:19 by , 15 years ago
Cc: | added |
---|
That is just so unfair that register.inclusion_tag does accept takes_context=True and register.simple_tag doesn't. That leads to simple yet inefficient tags that render from a template just to be able to access the context. (The alternative is to create monstrous full-blown Node classes which is an overkill in most situations).
comment:20 by , 15 years ago
i agree with semenov.
the reason i've found this ticket is because i was looking for a simpler way of accessing the context.
comment:22 by , 15 years ago
Considering my last attempt to add a bit of more usability to Django API (#11824), that's not going to happen. You can use this third-party implementation of simple_tag which has takes_context parameter: http://svn.navi.cx/misc/trunk/djblets/djblets/util/decorators.py
comment:23 by , 14 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Per design conversation with Malcolm, Russell, and Jannis at sprint, this is in essence accepted, but we're opening a new ticket (#14262) for better clarity.
Patch adds simple_tag_with_context tag function decorator