Opened 17 years ago

Closed 12 years ago

Last modified 12 years ago

#3100 closed New feature (fixed)

Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})

Reported by: Eric Van Dewoestine <ervandew@…> Owned by: Aymeric Augustin
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: Daniel Ring, mmitar@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Patch based on development version 4051.

Currently the template functionality prevents creation of tags which have intermediate tags requiring arguments.

For instance:

{% myif arg %}
  first block
{% elif arg %}  <-- current version would fail to parse this correctly because of the arg
  second block
{% endmyif %}

The attached patch removes this restriction.

Attachments (7)

__init__.py.diff (992 bytes ) - added by Eric Van Dewoestine <ervandew@…> 17 years ago.
patch-breaking-namedendblocks-in-regressiontests-templates.diff (1.1 KB ) - added by Daniel Ring 15 years ago.
patch breaks regressiontests.templates 'namedendblocks0[234]'
django_elif_docs.diff (817 bytes ) - added by cwebber 14 years ago.
Docs for the new {% elif %} tag
django_elif.diff (8.8 KB ) - added by cwebber 14 years ago.
Django {% elif %} tag support (now with docs in the same patch)
fix3100.patch (9.0 KB ) - added by Matthias Kestenholz 13 years ago.
3100-alternate.diff (1.3 KB ) - added by Chris Beaven 13 years ago.
An alternative to adding a parse_until_args_allowed argument
3100-backwards-compatible.patch (2.1 KB ) - added by Aymeric Augustin 12 years ago.

Download all attachments as: .zip

Change History (29)

by Eric Van Dewoestine <ervandew@…>, 17 years ago

Attachment: __init__.py.diff added

comment:1 by Eric Van Dewoestine <ervandew@…>, 17 years ago

Summary: Patch: Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})[patch]: Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})

comment:2 by Eric Van Dewoestine <ervandew@…>, 17 years ago

note patch is for file django/template/init.py.diff

comment:3 by Eric Van Dewoestine <ervandew@…>, 17 years ago

Resolution: duplicate
Status: newclosed

Just stumbled on http://code.djangoproject.com/ticket/3090 ... not sure which one is more or less correct, but as long as the referenced example in each is supported, I'm happy.

comment:4 by Russell Keith-Magee, 15 years ago

Needs tests: set
Patch needs improvement: set
Resolution: duplicate
Status: closedreopened
Summary: [patch]: Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})

This may be a duplicate of #3090, but this ticket describes the problem much better. Reopening.

comment:5 by Julien Phalip, 15 years ago

See also #8296 for a similar issue but formulated differently.

comment:6 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedAccepted

by Daniel Ring, 15 years ago

patch breaks regressiontests.templates 'namedendblocks0[234]'

comment:7 by Daniel Ring, 15 years ago

Cc: Daniel Ring added

The patch just added breaks the regressiontests.templates 'namedendblocks0?' by failing to raise TemplateSyntaxError when {% endblock %} tags are out of order. (Since endblock tags are the only (core) tags where the endtag has an argument.)

So two possibilities are:

  1. remove those tests - seems okay to me, since the named-endblock tags are for readability anyway, rather than providing functionality; also, named endblocks are only used (at least in trunk) in contrib.gis javascripts.
  2. make additional changes in template.loader_tags.do_block to force strict checking of endblock tags there

Which would be the desired route? Or something altogether different?

comment:8 by Adrian Holovaty, 14 years ago

I'm fine with removing those named-end-block tests. I agree that named endblock tags are just for readability, anyway.

comment:9 by cwebber, 14 years ago

I removed those named-end-block tests and also added {% elif %} tag support (yay, {% elif %} tags is a major feature!)

comment:10 by cwebber, 14 years ago

Needs tests: unset
Patch needs improvement: unset

by cwebber, 14 years ago

Attachment: django_elif_docs.diff added

Docs for the new {% elif %} tag

comment:11 by cwebber, 14 years ago

Triage Stage: AcceptedReady for checkin

Added docs for the {% elif %} tag.

by cwebber, 14 years ago

Attachment: django_elif.diff added

Django {% elif %} tag support (now with docs in the same patch)

comment:12 by Mitar, 14 years ago

Cc: mmitar@… added

comment:13 by Malcolm Tredinnick, 14 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Hmm... so this is two different things in one patch: (1) is allowing args on the intermediate tag, (2) changing if tag to include elif. We might want (1) without (2), hence our usual insistence on one feature per ticket.

That being said, the patch looks reasonable. However, the change to the tests to remove the unbalanced blocks test is bad. You are removing tests that check for something bad happening. If this patch doesn't let those tests still fail, the patch is broken. If it doesn't affect those tests, they should remain. Either way, this isn't ready for checkin yet. I'll add "needs tests" to the flags for this reason, in case somebody is looking for test related work. It doesn't need new tests, just those deleted "extra endblock" tests restored, please.

comment:14 by Matthias Kestenholz, 13 years ago

Needs documentation: set
Needs tests: unset

A differentiation between intermediate tags which accept arguments and tags who don't seems to be necessary to support this use case properly (without breaking backwards-compatibility or deleting tests).

The attached patch adds another argument to django.template.Parser.parse; parse_until is complemented by parse_until_args_allowed. The included if-tag supporting {% elif %} makes use of this. The previously deleted "extra endblock" tests have been restored and they pass, as they should.

The documentation about writing custom tags would have to be extended for this, though.

by Matthias Kestenholz, 13 years ago

Attachment: fix3100.patch added

comment:15 by Łukasz Rekucki, 13 years ago

Severity: normalNormal
Type: enhancementNew feature

by Chris Beaven, 13 years ago

Attachment: 3100-alternate.diff added

An alternative to adding a parse_until_args_allowed argument

comment:16 by Cal Leeming, 12 years ago

Easy pickings: unset
UI/UX: unset

Guys - any idea when this might make it into a release??

Cal

comment:17 by Aymeric Augustin, 12 years ago

#17348 was created to discuss the addition of {% elif %} — as pointed out by Malcolm these are two different (however related) features.

comment:18 by Aymeric Augustin, 12 years ago

Owner: changed from Adrian Holovaty to Aymeric Augustin
Status: reopenednew

comment:19 by Aymeric Augustin, 12 years ago

Here are some implementation details of the Parser class that I had to look up in the source in order to understand this ticket.

An instance of Parser holds a list of tokens — the output of Lexer.tokenize — in its tokens instance variable. Its parse method builds a list of nodes (nodelist) by repeatedly popping the first token, compiling it, and appending to nodelist appropriate nodes.

parse terminates:

  • when it reaches the end of the token stream;
  • when it receives a block token whose content is found in parse_until (when it was called with a parse_until argument).

parse_until is used by tags that use final (or intermediary) tags, like {% block ... %} {% endblock %}, or {% for ..%} {% empty %} {% endfor %}. However, the current implementation requires that the exact content of the final (or intermediary) tag is known by the time the initial tag is compiled. This makes it impossible to have variable arguments on the intermediary tag.

This feature is used to reject unmatched block/endblock tags: {% block foo %}{% endblock bar %}. I think this check could be delegated to the block tag itself. The alternative is to use Chris' technique — looking for an exact match by default, and for a prefix match for elements that end with a space — but it feels hackish.

Last edited 12 years ago by Aymeric Augustin (previous) (diff)

comment:20 by Aymeric Augustin, 12 years ago

Attached patch implement changes accept arguments on intermediary or final tags in a backwards compatible way.

This is exercised by the new implementation of the {% block %} tag. Namely, template test namedendblocks04 breaks if the changes in django/template/loader_tags.py are omitted.

by Aymeric Augustin, 12 years ago

comment:21 by Aymeric Augustin, 12 years ago

Resolution: fixed
Status: newclosed

In [17186]:

Fixed #3100 -- Added support for arguments on intermediate tag tokens.

comment:22 by Aymeric Augustin, 12 years ago

In [17187]:

Fixed #17348 -- Implemented {% elif %}. Refs #3100.

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