#3100 closed New feature (fixed)
Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})
Reported by: | 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)
Change History (29)
by , 18 years ago
Attachment: | __init__.py.diff added |
---|
comment:1 by , 18 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 , 18 years ago
comment:3 by , 18 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
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 , 15 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Resolution: | duplicate |
Status: | closed → reopened |
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:6 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
by , 15 years ago
Attachment: | patch-breaking-namedendblocks-in-regressiontests-templates.diff added |
---|
patch breaks regressiontests.templates 'namedendblocks0[234]'
comment:7 by , 15 years ago
Cc: | 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:
- 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.
- 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 , 15 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 , 15 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 , 15 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:11 by , 15 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Added docs for the {% elif %} tag.
by , 15 years ago
Attachment: | django_elif.diff added |
---|
Django {% elif %} tag support (now with docs in the same patch)
comment:12 by , 15 years ago
Cc: | added |
---|
comment:13 by , 14 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
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 , 14 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 , 14 years ago
Attachment: | fix3100.patch added |
---|
comment:15 by , 14 years ago
Severity: | normal → Normal |
---|---|
Type: | enhancement → New feature |
by , 14 years ago
Attachment: | 3100-alternate.diff added |
---|
An alternative to adding a parse_until_args_allowed
argument
comment:16 by , 13 years ago
Easy pickings: | unset |
---|---|
UI/UX: | unset |
Guys - any idea when this might make it into a release??
Cal
comment:17 by , 13 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 , 13 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
comment:19 by , 13 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 aparse_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.
comment:20 by , 13 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 , 13 years ago
Attachment: | 3100-backwards-compatible.patch added |
---|
note patch is for file django/template/init.py.diff