Opened 5 years ago

Closed 6 months ago

#13956 closed New feature (fixed)

Indefinite args for simpletags and inclusion tags

Reported by: melinath Owned by: julien
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 5 years ago.
Patch
indefinite_inclusion_tag_args_with_tests.diff (7.3 KB) - added by gregmuellegger 5 years ago.
Adding tests to previous patch.
indefinite_args_patch_with_complete_tests.diff (10.2 KB) - added by melinath 5 years ago.
patch_r15283.diff (10.7 KB) - added by melinath 4 years ago.
patch_r16322.diff (10.9 KB) - added by melinath 4 years ago.
13956r16406.diff (13.4 KB) - added by melinath 4 years ago.
13956r16423.diff (25.9 KB) - added by melinath 4 years ago.
13956.ttag_helpers_args_kwargs_support.diff (50.0 KB) - added by julien 4 years ago.
13956.ttag_helpers_args_kwargs_support.2.diff (73.2 KB) - added by jezdez 4 years ago.
Updated patch with PEP8 fixes.
13956.ttag_helpers_args_kwargs_support.3.diff (82.5 KB) - added by julien 4 years ago.
Final patch. Will commit shortly.
13956.ttag_varargs_in_templates.1.diff (9.4 KB) - added by pczerkas 6 months ago.

Download all attachments as: .zip

Change History (47)

Changed 5 years ago by melinath

Patch

comment:1 Changed 5 years ago by gregmuellegger

  • Needs documentation set
  • Needs tests set
  • Owner changed from nobody to gregmuellegger
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 5 years ago by russellm

  • Triage Stage changed from Design decision needed to Accepted

Changed 5 years ago by gregmuellegger

Adding tests to previous patch.

comment:3 Changed 5 years ago by gregmuellegger

  • Keywords simple_tag simpletag added
  • Needs tests unset
  • Version changed from 1.2 to SVN

comment:4 Changed 5 years ago by gregmuellegger

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

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

comment:7 Changed 4 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 4 years ago by graham_king

  • Severity set to Normal
  • Type set to New feature

comment:9 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

patch_r15283.diff fails to apply cleanly on to trunk

comment:10 Changed 4 years ago by melinath

  • Patch needs improvement unset

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

Changed 4 years ago by melinath

comment:11 Changed 4 years ago by julien

  • 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 4 years ago by melinath

comment:12 Changed 4 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 4 years ago by julien

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 4 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 4 years ago by julien

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

comment:18 Changed 4 years ago by melinath

Incidentally, passes with no unexpected failures.

comment:19 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

That looks great, thank you!

comment:20 Changed 4 years ago by jezdez

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 4 years ago by julien

  • Triage Stage changed from Ready for checkin to 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 Changed 4 years ago by jezdez

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

comment:23 Changed 4 years ago by SamBull

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 4 years ago by jezdez (previous) (diff)

comment:24 Changed 4 years ago by EnTeQuAk

  • Cc cg@… added

comment:25 Changed 4 years ago by melinath

  • Owner changed from gregmuellegger to melinath
  • Status changed from assigned to new
  • Triage Stage changed from Accepted to 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 Changed 4 years ago by SamBull

  • Cc osirius@… added

comment:27 Changed 4 years ago by julien

  • Owner changed from melinath to julien
  • Triage Stage changed from Design decision needed to Accepted

I'm working on this.

Changed 4 years ago by julien

comment:28 Changed 4 years ago by julien

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 4 years ago by julien

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 4 years ago by jezdez

Updated patch with PEP8 fixes.

comment:30 Changed 4 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

LGTM, thanks!

comment:31 Changed 4 years ago by julien

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

Changed 4 years ago by julien

Final patch. Will commit shortly.

comment:32 Changed 4 years ago by julien

  • Resolution set to fixed
  • Status changed from new to closed

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 12 months 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 6 months ago by pczerkas

comment:34 Changed 6 months ago by pczerkas

  • Cc pczerkas added
  • Keywords varargs added
  • Resolution fixed deleted
  • Status changed from closed to 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 Changed 6 months ago by collinanderson

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 6 months ago by timgraham

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top