Opened 14 years ago
Closed 10 years ago
#13956 closed New feature (fixed)
Indefinite args for simpletags and inclusion tags
Reported by: | Stephen Burrows | Owned by: | Julien Phalip |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | simple_tag, simpletag, varargs |
Cc: | stephen.r.burrows@…, cg@…, osirius@…, pczerkas | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This patch lets simple tags and inclusion tags accept *args.
For comparison: Currently, an inclusion tag must define an exact number of args. In some cases, such as a tag created with a wrapped function, the args taken by the inner function may not be known; with this patch, that wouldn't matter.
Attachments (11)
Change History (47)
by , 14 years ago
Attachment: | indefinite_inclusion_tag_args.diff added |
---|
comment:1 by , 14 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Owner: | changed from | to
Status: | new → assigned |
Triage Stage: | Unreviewed → Design decision needed |
For reference, the mailinglist discussion: http://groups.google.com/group/django-developers/browse_thread/thread/abdbe6e563ec8e29
comment:2 by , 14 years ago
Triage Stage: | Design decision needed → Accepted |
---|
by , 14 years ago
Attachment: | indefinite_inclusion_tag_args_with_tests.diff added |
---|
Adding tests to previous patch.
comment:3 by , 14 years ago
Keywords: | simple_tag simpletag added |
---|---|
Needs tests: | unset |
Version: | 1.2 → SVN |
comment:4 by , 14 years ago
Needs documentation: | unset |
---|---|
Needs tests: | set |
I just reviewed the simple tag documentation and saw that there is no need to change the documentation - there is no mention of the limitation that *args might not be allowed. Or should this be pointed out in the docs?
Tests for inclusion tag are missing, I try to write these in the next time.
by , 14 years ago
Attachment: | indefinite_args_patch_with_complete_tests.diff added |
---|
comment:5 by , 14 years ago
Needs tests: | unset |
---|
indefinite_args_patch_with_complete_tests.diff changes the following:
- Moves simple_tag and inclusion_tag tests in with the other template tests to take advantage of the framework. This is especially helpful for the inclusion tags since it means the test loader is accessible.
- Adds inclusion_tag tests.
- Adds tests for simple tags and inclusion tags that only have *args.
I did not add tests for takes_context=True... I can add those fairly easily later if they are necessary. Which I guess they probably are?
comment:6 by , 14 years ago
Cc: | added |
---|
Updated the patch to be off of r15283. There are no test failures with the patch that are not there without the patch.
by , 14 years ago
Attachment: | patch_r15283.diff added |
---|
comment:7 by , 14 years ago
Added a git branch for this ticket @ https://github.com/melinath/django/tree/ticket13956. Patch is still current as of r15353.
comment:8 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:9 by , 14 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | set |
patch_r15283.diff fails to apply cleanly on to trunk
comment:10 by , 13 years ago
Patch needs improvement: | unset |
---|
Updated the patch to r16322. Applies cleanly with no unexpected test failures.
by , 13 years ago
Attachment: | patch_r16322.diff added |
---|
comment:11 by , 13 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
UI/UX: | unset |
Thank you, the patch looks pretty good! For consistency, could the same be done for @assignment_tag
? The documentation also needs to be updated.
by , 13 years ago
Attachment: | 13956r16406.diff added |
---|
comment:12 by , 13 years ago
Needs tests: | set |
---|
I added varargs support to the assignment tag, but I still need to add tests for it. Also, I rearranged things a little, which could cause backwards incompatibility with anyone using internals extremely heavily. It's not necessary and I could revert that bit if people think it will cause serious problems.
What exactly needs to be documented? For me, this is the expected behavior.
comment:13 by , 13 years ago
From what I see in the patch I don't think backwards incompatibility would be an issue, especially as this is really low level internal stuff. Can you think of specific ways where that would be an issue?
For the doc, a short notice in each section for include_tag, simple_tag and assignment_tag saying that those tags can have an indefinite number of arguments would suffice. It's also worth mentioning this new feature in the release notes.
comment:14 by , 13 years ago
Specifically, the patch in its current state changes the signature of generic_tag_compiler from (params, defaults, name, node_class, parser, token) to (parser, token, params, defaults, name, node_class, varargs) for no reason other than that I think it reads better to have parser and token first, since that's standard for tag compilation functions anyway. If anyone were using generic_tag_compiler and counting on the order of the arguments, this patch would break it.
I'll add the tests, docs and release notes to the patch soonish. (Unless someone beats me to it.)
comment:15 by , 13 years ago
OK, so if it's purely for cosmetic reasons, let's just leave it the way it was. I don't think the change would affect many people but it's not really worth taking any risk here.
comment:16 by , 13 years ago
On the other hand, "beautiful is better than ugly". If I really want to maintain backwards-compatibility, I need to make the additional argument optional. Comparison:
- Original signature:
(params, defaults, name, node_class, parser, token)
- New signature:
(parser, token, params, defaults, name, node_class, varargs)
- Alternate:
(params, defaults, name, node_class, varargs, parser, token)
- Backwards-compatible signature:
(params, defaults, name, node_class, parser, token, varargs=None)
4 is taking a turn down an inelegant path. I really would rather do 2 or 3.
comment:17 by , 13 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
This patch should address all concerns. Has docs and release notes. Has tests for everything - now modeled after and together with the other simple, inclusion, and assignment tests instead of elsewhere. Uses the backwards-compatible signature.
by , 13 years ago
Attachment: | 13956r16423.diff added |
---|
comment:19 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
That looks great, thank you!
comment:20 by , 13 years ago
While I appreciate the effort of this ticket I'd like to voice my concern that adding support for *args
but not **kwargs
might not be the wisest decision with regard to compatibility. We now have a good standard for kwargs in template tags (due to the with/include tag refactor by SmileyChris) and shouldn't ignore this when extending the helper tags.
comment:21 by , 13 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Sorry, after discussing with jezdez on IRC, we're going to hold off for the moment. The idea is to investigate whether support for **kwargs
should also be added at the same time, while we're at it. Any insight on the implementation details for that would be useful.
comment:22 by , 13 years ago
Thanks Julien, much appreciated. I wrote to the -developers list about it, too.
comment:23 by , 13 years ago
I built a utility app called fancy_tag that addresses this issue. It's a drop-in replacement for simple_tag that adds support for *args
, keyword args, **kwargs
. It also adds support for the "with <varname>" pattern. The app comes with unit tests and the whole thing shouldn't be too hard to convert into a patch.
You can check it out here: https://github.com/trapeze/fancy_tag
Let me know if you'd like me to submit this as a patch.
comment:24 by , 13 years ago
Cc: | added |
---|
comment:25 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
Triage Stage: | Accepted → Design decision needed |
Reassigning to myself after asking gregmuelleger. Also setting to DDN since that seems to be the case. See the discussion in the developer's list.
comment:26 by , 13 years ago
Cc: | added |
---|
comment:27 by , 13 years ago
Owner: | changed from | to
---|---|
Triage Stage: | Design decision needed → Accepted |
I'm working on this.
by , 13 years ago
Attachment: | 13956.ttag_helpers_args_kwargs_support.diff added |
---|
comment:28 by , 13 years ago
OK, so I think this is ready for review. I've got some inspiration from SamBull's fancy_tag
, although the implementation here is a bit cleaner and a bit more robust. It's quite thoroughly tested. I still need to write some documentation. Any feedback welcome, thank you!
comment:29 by , 13 years ago
Oh, I should have clarified that with the proposed patch inclusion_tag
, simple_tag
and assignment_tag
now have full *args
and **kwargs
support, working the same way as in Python.
by , 13 years ago
Attachment: | 13956.ttag_helpers_args_kwargs_support.2.diff added |
---|
Updated patch with PEP8 fixes.
comment:31 by , 13 years ago
Thanks a lot for the review and PEP8 cleanup, Jannis! I'll write a bit of documentation before pushing it in.
by , 13 years ago
Attachment: | 13956.ttag_helpers_args_kwargs_support.3.diff added |
---|
Final patch. Will commit shortly.
comment:33 by , 10 years ago
I think that one piece is still missing. How about allowing to pass args/kwargs to templatetags:
{% some_tag named_args *args **kwargs %}
where args is a list, kwargs is a dict?
by , 10 years ago
Attachment: | 13956.ttag_varargs_in_templates.1.diff added |
---|
comment:34 by , 10 years ago
Cc: | added |
---|---|
Keywords: | varargs added |
Resolution: | fixed |
Status: | closed → new |
Replying to pczerkas@…:
I think that one piece is still missing. How about allowing to pass args/kwargs to templatetags:
{% some_tag named_args *args **kwargs %}where args is a list, kwargs is a dict?
Patch for proposed functionality: https://code.djangoproject.com/attachment/ticket/13956/13956.ttag_varargs_in_templates.1.diff with tests.
The mailinglist discussion: https://groups.google.com/forum/#!topic/django-developers/IU128CmM9Es
comment:35 by , 10 years ago
Hi! Thanks for the patch. Could you open a separate ticket for this and maybe link the two together because they're related?
comment:36 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Patch