Opened 19 years ago

Closed 14 years ago

#1105 closed defect (duplicate)

[patch] simple_tag decorator enhancement

Reported by: django@… 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)

simple_tag_with_context_r1764.diff (1.2 KB ) - added by django@… 19 years ago.
Patch adds simple_tag_with_context tag function decorator
simple_tag_r1785.diff (3.1 KB ) - added by django@… 19 years ago.
simple_tag enhancement
super_simple_tag.diff (11.7 KB ) - added by Julien Phalip 16 years ago.
Patch + tests + doc (created from revision 7774)
1105.simple_tag.r7813.diff (13.3 KB ) - added by Johannes Dollinger 16 years ago.
takes_nodelist instead of takes_block
improved_simple_tag.r7818.diff (11.7 KB ) - added by Julien Phalip 16 years ago.
Replaced "takes_block" by "takes_nodelist", and "block_nodelist" by "nodelist". Amended the doc and tests as well.
1105.simple_tag.diff (11.8 KB ) - added by Julien Phalip 16 years ago.
Updated patch to r8984
1105.simple_tag.r9050.diff (11.8 KB ) - added by Julien Phalip 16 years ago.
Updated patch to r9050

Download all attachments as: .zip

Change History (30)

by django@…, 19 years ago

Patch adds simple_tag_with_context tag function decorator

comment:1 by rjwittams, 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 django@…, 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;).

by django@…, 19 years ago

Attachment: simple_tag_r1785.diff added

simple_tag enhancement

comment:3 by URL, 19 years ago

Type: enhancement

comment:4 by Malcolm Tredinnick, 18 years ago

Owner: changed from Adrian Holovaty to Malcolm Tredinnick
Type: defect

comment:5 by Chris Beaven, 18 years ago

Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedAccepted

Seems like a useful enhancement.

comment:6 by racter, 17 years ago

Patch needs improvement: set

patch doesn't apply anymore

comment:7 by Julien Phalip, 16 years ago

Cc: django@… added
Component: Core frameworkTemplate system
Owner: changed from nobody to Julien Phalip

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):

comment:8 by Julien Phalip, 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?

in reply to:  8 comment:9 by Johannes Dollinger, 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 Julien Phalip, 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 Justin, 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 Alex Gaynor, 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 Julien Phalip, 16 years ago

Attachment: super_simple_tag.diff added

Patch + tests + doc (created from revision 7774)

comment:13 by Julien Phalip, 16 years ago

Thanks Alex and justinf, I've applied your suggestions in the new patch.

by Johannes Dollinger, 16 years ago

Attachment: 1105.simple_tag.r7813.diff added

takes_nodelist instead of takes_block

comment:14 by Johannes Dollinger, 16 years ago

I'd prefer a takes_nodelist argument since you will get a NodeList that has nothing to do with {% block %}.

by Julien Phalip, 16 years ago

Replaced "takes_block" by "takes_nodelist", and "block_nodelist" by "nodelist". Amended the doc and tests as well.

comment:15 by Julien Phalip, 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:16 by timolux, 16 years ago

What about also allowing for variable number of args?

by Julien Phalip, 16 years ago

Attachment: 1105.simple_tag.diff added

Updated patch to r8984

by Julien Phalip, 16 years ago

Attachment: 1105.simple_tag.r9050.diff added

Updated patch to r9050

comment:17 by Julien Phalip, 16 years ago

Triage Stage: AcceptedDesign 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 Skaffen, 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 Ilya Semenov, 15 years ago

Cc: semenov@… 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 twig, 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:21 by valentin golev, 15 years ago

So, will it be added to trunk?

comment:22 by Ilya Semenov, 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 Carl Meyer, 14 years ago

Resolution: duplicate
Status: newclosed

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.

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