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)

indefinite_inclusion_tag_args.diff (3.3 KB ) - added by Stephen Burrows 14 years ago.
Patch
indefinite_inclusion_tag_args_with_tests.diff (7.3 KB ) - added by Gregor Müllegger 14 years ago.
Adding tests to previous patch.
indefinite_args_patch_with_complete_tests.diff (10.2 KB ) - added by Stephen Burrows 14 years ago.
patch_r15283.diff (10.7 KB ) - added by Stephen Burrows 14 years ago.
patch_r16322.diff (10.9 KB ) - added by Stephen Burrows 13 years ago.
13956r16406.diff (13.4 KB ) - added by Stephen Burrows 13 years ago.
13956r16423.diff (25.9 KB ) - added by Stephen Burrows 13 years ago.
13956.ttag_helpers_args_kwargs_support.diff (50.0 KB ) - added by Julien Phalip 13 years ago.
13956.ttag_helpers_args_kwargs_support.2.diff (73.2 KB ) - added by Jannis Leidel 13 years ago.
Updated patch with PEP8 fixes.
13956.ttag_helpers_args_kwargs_support.3.diff (82.5 KB ) - added by Julien Phalip 13 years ago.
Final patch. Will commit shortly.
13956.ttag_varargs_in_templates.1.diff (9.4 KB ) - added by pczerkas 10 years ago.

Download all attachments as: .zip

Change History (47)

by Stephen Burrows, 14 years ago

Patch

comment:1 by Gregor Müllegger, 14 years ago

Needs documentation: set
Needs tests: set
Owner: changed from nobody to Gregor Müllegger
Status: newassigned
Triage Stage: UnreviewedDesign decision needed

comment:2 by Russell Keith-Magee, 14 years ago

Triage Stage: Design decision neededAccepted

by Gregor Müllegger, 14 years ago

Adding tests to previous patch.

comment:3 by Gregor Müllegger, 14 years ago

Keywords: simple_tag simpletag added
Needs tests: unset
Version: 1.2SVN

comment:4 by Gregor Müllegger, 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 Stephen Burrows, 14 years ago

comment:5 by Stephen Burrows, 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 Stephen Burrows, 14 years ago

Cc: stephen.r.burrows@… 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 Stephen Burrows, 14 years ago

Attachment: patch_r15283.diff added

comment:7 by Stephen Burrows, 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 Graham King, 14 years ago

Severity: Normal
Type: New feature

comment:9 by patchhammer, 14 years ago

Easy pickings: unset
Patch needs improvement: set

patch_r15283.diff fails to apply cleanly on to trunk

comment:10 by Stephen Burrows, 13 years ago

Patch needs improvement: unset

Updated the patch to r16322. Applies cleanly with no unexpected test failures.

by Stephen Burrows, 13 years ago

Attachment: patch_r16322.diff added

comment:11 by Julien Phalip, 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 Stephen Burrows, 13 years ago

Attachment: 13956r16406.diff added

comment:12 by Stephen Burrows, 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 Julien Phalip, 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 Stephen Burrows, 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 Julien Phalip, 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 Stephen Burrows, 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:

  1. Original signature: (params, defaults, name, node_class, parser, token)
  2. New signature: (parser, token, params, defaults, name, node_class, varargs)
  3. Alternate: (params, defaults, name, node_class, varargs, parser, token)
  4. 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 Stephen Burrows, 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 Stephen Burrows, 13 years ago

Attachment: 13956r16423.diff added

comment:18 by Stephen Burrows, 13 years ago

Incidentally, passes with no unexpected failures.

comment:19 by Julien Phalip, 13 years ago

Triage Stage: AcceptedReady for checkin

That looks great, thank you!

comment:20 by Jannis Leidel, 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 Julien Phalip, 13 years ago

Triage Stage: Ready for checkinAccepted

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 Jannis Leidel, 13 years ago

Thanks Julien, much appreciated. I wrote to the -developers list about it, too.

comment:23 by Sam Bull, 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.

Last edited 13 years ago by Jannis Leidel (previous) (diff)

comment:24 by Christopher Grebs, 13 years ago

Cc: cg@… added

comment:25 by Stephen Burrows, 13 years ago

Owner: changed from Gregor Müllegger to Stephen Burrows
Status: assignednew
Triage Stage: AcceptedDesign 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 Sam Bull, 13 years ago

Cc: osirius@… added

comment:27 by Julien Phalip, 13 years ago

Owner: changed from Stephen Burrows to Julien Phalip
Triage Stage: Design decision neededAccepted

I'm working on this.

by Julien Phalip, 13 years ago

comment:28 by Julien Phalip, 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 Julien Phalip, 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 Jannis Leidel, 13 years ago

Updated patch with PEP8 fixes.

comment:30 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

LGTM, thanks!

comment:31 by Julien Phalip, 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 Julien Phalip, 13 years ago

Final patch. Will commit shortly.

comment:32 by Julien Phalip, 13 years ago

Resolution: fixed
Status: newclosed

In [16908]:

Fixed #13956 -- Enabled *args and **kwargs support for simple_tag, inclusion_tag and assignment_tag. Many thanks to Stephen Burrows for the report and initial patch, to Gregor Müllegger for the initial tests, to SamBull for the suggestions, and to Jannis Leidel for the review and PEP8 cleanup.

comment:33 by pczerkas@…, 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?

comment:34 by pczerkas, 10 years ago

Cc: pczerkas added
Keywords: varargs added
Resolution: fixed
Status: closednew

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 Collin Anderson, 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 Tim Graham, 10 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top