Opened 6 years ago

Closed 2 years ago

#13956 closed New feature (fixed)

Indefinite args for simpletags and inclusion tags

Reported by: melinath Owned by: Julien Phalip
Component: Template system Version: master
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 melinath 6 years ago.
Patch
indefinite_inclusion_tag_args_with_tests.diff (7.3 KB) - added by Gregor Müllegger 6 years ago.
Adding tests to previous patch.
indefinite_args_patch_with_complete_tests.diff (10.2 KB) - added by melinath 6 years ago.
patch_r15283.diff (10.7 KB) - added by melinath 6 years ago.
patch_r16322.diff (10.9 KB) - added by melinath 6 years ago.
13956r16406.diff (13.4 KB) - added by melinath 5 years ago.
13956r16423.diff (25.9 KB) - added by melinath 5 years ago.
13956.ttag_helpers_args_kwargs_support.diff (50.0 KB) - added by Julien Phalip 5 years ago.
13956.ttag_helpers_args_kwargs_support.2.diff (73.2 KB) - added by Jannis Leidel 5 years ago.
Updated patch with PEP8 fixes.
13956.ttag_helpers_args_kwargs_support.3.diff (82.5 KB) - added by Julien Phalip 5 years ago.
Final patch. Will commit shortly.
13956.ttag_varargs_in_templates.1.diff (9.4 KB) - added by pczerkas 2 years ago.

Download all attachments as: .zip

Change History (47)

Changed 6 years ago by melinath

Patch

comment:1 Changed 6 years ago by Gregor Müllegger

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

comment:2 Changed 6 years ago by Russell Keith-Magee

Triage Stage: Design decision neededAccepted

Changed 6 years ago by Gregor Müllegger

Adding tests to previous patch.

comment:3 Changed 6 years ago by Gregor Müllegger

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

comment:4 Changed 6 years ago by Gregor Müllegger

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.

Changed 6 years ago by melinath

comment:5 Changed 6 years ago by melinath

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 Changed 6 years ago by melinath

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.

Changed 6 years ago by melinath

Attachment: patch_r15283.diff added

comment:7 Changed 6 years ago by melinath

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

comment:8 Changed 6 years ago by Graham King

Severity: Normal
Type: New feature

comment:9 Changed 6 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set

patch_r15283.diff fails to apply cleanly on to trunk

comment:10 Changed 6 years ago by melinath

Patch needs improvement: unset

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

Changed 6 years ago by melinath

Attachment: patch_r16322.diff added

comment:11 Changed 5 years ago by Julien Phalip

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.

Changed 5 years ago by melinath

Attachment: 13956r16406.diff added

comment:12 Changed 5 years ago by melinath

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 Changed 5 years ago by 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.

comment:14 Changed 5 years ago by melinath

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 Changed 5 years ago by 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.

comment:16 Changed 5 years ago by melinath

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 Changed 5 years ago by melinath

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.

Changed 5 years ago by melinath

Attachment: 13956r16423.diff added

comment:18 Changed 5 years ago by melinath

Incidentally, passes with no unexpected failures.

comment:19 Changed 5 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

That looks great, thank you!

comment:20 Changed 5 years ago by 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.

comment:21 Changed 5 years ago by Julien Phalip

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 Changed 5 years ago by Jannis Leidel

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

comment:23 Changed 5 years ago by 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.

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

comment:24 Changed 5 years ago by Christopher Grebs

Cc: cg@… added

comment:25 Changed 5 years ago by melinath

Owner: changed from Gregor Müllegger to melinath
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 Changed 5 years ago by Sam Bull

Cc: osirius@… added

comment:27 Changed 5 years ago by Julien Phalip

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

I'm working on this.

Changed 5 years ago by Julien Phalip

comment:28 Changed 5 years ago by 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!

comment:29 Changed 5 years ago by 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.

Changed 5 years ago by Jannis Leidel

Updated patch with PEP8 fixes.

comment:30 Changed 5 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

LGTM, thanks!

comment:31 Changed 5 years ago by Julien Phalip

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

Changed 5 years ago by Julien Phalip

Final patch. Will commit shortly.

comment:32 Changed 5 years ago by Julien Phalip

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 Changed 3 years ago by 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?

Changed 2 years ago by pczerkas

comment:34 Changed 2 years ago by pczerkas

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 Changed 2 years ago by 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?

comment:36 Changed 2 years ago by Tim Graham

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