Code

Opened 9 years ago

Closed 4 years ago

#1105 closed defect (duplicate)

[patch] simple_tag decorator enhancement

Reported by: django@… Owned by: julien
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: UI/UX:

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@… 9 years ago.
Patch adds simple_tag_with_context tag function decorator
simple_tag_r1785.diff (3.1 KB) - added by django@… 9 years ago.
simple_tag enhancement
super_simple_tag.diff (11.7 KB) - added by julien 6 years ago.
Patch + tests + doc (created from revision 7774)
1105.simple_tag.r7813.diff (13.3 KB) - added by emulbreh 6 years ago.
takes_nodelist instead of takes_block
improved_simple_tag.r7818.diff (11.7 KB) - added by julien 6 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 6 years ago.
Updated patch to r8984
1105.simple_tag.r9050.diff (11.8 KB) - added by julien 6 years ago.
Updated patch to r9050

Download all attachments as: .zip

Change History (30)

Changed 9 years ago by django@…

Patch adds simple_tag_with_context tag function decorator

comment:1 Changed 9 years ago by rjwittams

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 Changed 9 years ago by django@…

  • Summary changed from [patch] simple_tag_with_context decorator to [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;).

Changed 9 years ago by django@…

simple_tag enhancement

comment:3 Changed 8 years ago by URL

  • Type enhancement deleted

comment:4 Changed 8 years ago by mtredinnick

  • Owner changed from adrian to mtredinnick
  • Type set to defect

comment:5 Changed 7 years ago by SmileyChris

  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Seems like a useful enhancement.

comment:6 Changed 7 years ago by racter

  • Patch needs improvement set

patch doesn't apply anymore

comment:7 Changed 6 years ago by julien

  • Cc django@… added
  • Component changed from Core framework to Template system
  • Owner changed from nobody to julien

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 follow-up: Changed 6 years ago by julien

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 in reply to: ↑ 8 Changed 6 years ago by emulbreh

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 Changed 6 years ago by julien

Thanks emulbreh, well spotted!
I have updated the patch, which is now fully fonctional. Will post documentation and test shortly.

comment:11 Changed 6 years ago by justinf

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 Changed 6 years ago by Alex

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.

Changed 6 years ago by julien

Patch + tests + doc (created from revision 7774)

comment:13 Changed 6 years ago by julien

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

Changed 6 years ago by emulbreh

takes_nodelist instead of takes_block

comment:14 Changed 6 years ago by emulbreh

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

Changed 6 years ago by julien

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

comment:15 Changed 6 years ago by julien

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 Changed 6 years ago by timolux

What about also allowing for variable number of args?

Changed 6 years ago by julien

Updated patch to r8984

Changed 6 years ago by julien

Updated patch to r9050

comment:17 Changed 6 years ago by julien

  • Triage Stage changed from Accepted to 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 Changed 6 years ago by Skaffen

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 Changed 5 years ago by semenov

  • 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 Changed 5 years ago by twig

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 Changed 4 years ago by valentin golev

So, will it be added to trunk?

comment:22 Changed 4 years ago by semenov

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 Changed 4 years ago by carljm

  • Resolution set to duplicate
  • Status changed from new to 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.