Indefinite args for simpletags and inclusion tags

[Stephen Burrows]

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.

[Stephen Burrows]

Patch

[Gregor Müllegger]

For reference, the mailinglist discussion: ​http://groups.google.com/group/django-developers/browse_thread/thread/abdbe6e563ec8e29

[Gregor Müllegger]

Adding tests to previous patch.

[Gregor Müllegger]

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.

[Stephen Burrows]

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?

[Stephen Burrows]

Updated the patch to be off of r15283. There are no test failures with the patch that are not there without the patch.

[Stephen Burrows]

Added a git branch for this ticket @ ​https://github.com/melinath/django/tree/ticket13956. Patch is still current as of r15353.

[patchhammer]

patch_r15283.diff fails to apply cleanly on to trunk

[Stephen Burrows]

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

[Julien Phalip]

Thank you, the patch looks pretty good! For consistency, could the same be done for @assignment_tag? The documentation also needs to be updated.

[Stephen Burrows]

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.

[Julien Phalip]

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.

[Stephen Burrows]

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.)

[Julien Phalip]

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.

[Stephen Burrows]

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.

[Stephen Burrows]

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.

[Stephen Burrows]

Incidentally, passes with no unexpected failures.

[Julien Phalip]

That looks great, thank you!

[Jannis Leidel]

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.

[Julien Phalip]

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.

[Jannis Leidel]

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

[Sam Bull]

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.

[Stephen Burrows]

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.

[Julien Phalip]

I'm working on this.

[Julien Phalip]

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!

[Julien Phalip]

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.

[Jannis Leidel]

Updated patch with PEP8 fixes.

[Jannis Leidel]

LGTM, thanks!

[Julien Phalip]

Thanks a lot for the review and PEP8 cleanup, Jannis! I'll write a bit of documentation before pushing it in.

[Julien Phalip]

Final patch. Will commit shortly.

[Julien Phalip]

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.

[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?

[pczerkas]

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

[Collin Anderson]

Hi! Thanks for the patch. Could you open a separate ticket for this and maybe link the two together because they're related?