Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#3683 closed (wontfix)

permalink decorator takes redundant arguments

Reported by: Chris Beaven Owned by: Adrian Holovaty
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


In fixing the documentation for permalink (#3540) I realised that it is redundant to have to supply both args and kwargs to the decorator since reverse can't use a mix of ordered and named arguments.

Even though it's a backwards incompatible change, the format of the permalink decorator should change in my opinion.

Attachments (2)

better_permalink.patch (2.8 KB) - added by Chris Beaven 10 years ago.
better_permalink.2.patch (2.8 KB) - added by Chris Beaven 10 years ago.
Uh, I meant this one

Download all attachments as: .zip

Change History (18)

Changed 10 years ago by Chris Beaven

Attachment: better_permalink.patch added

Changed 10 years ago by Chris Beaven

Attachment: better_permalink.2.patch added

Uh, I meant this one

comment:1 Changed 10 years ago by Chris Beaven

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Actually, scratch that comment from the ticket about backwards incompatibility - I made it fully backwards compatible!

comment:2 Changed 10 years ago by Malcolm Tredinnick

I'm not convinced this is necessary. The patch replaces two lines of code with 18 lines of code. However, if we change nothing except the documentation, it remains backwards compatible and people just starting to use this will only pass in argument tuples. We can even add a one line comment saying why kwargs doesn't work as expected. Is there something else that this patch is doing? Not closing as wontfix yet, because I may be missing something.

comment:3 Changed 10 years ago by Chris Beaven

At the moment, you have to pass both args and kwargs (if you want to use kwargs) which is pretty redundant. I guess it is a bit of code creep though.

comment:4 Changed 10 years ago by Malcolm Tredinnick

OK, now I really don't understand what the bug is here. The function that is going to be decorated with permalink() needs to return a two- or three-tuple, where the optional third element is a dictionary of keyword args when calling the view. So, yes, if you want to use the third argument and pass in no positional args, the positional arg element must be an empty sequence. We can document that. It's not a bug, though.

Secondly, you say reverse() cannot use a mixture of positional and keyword args, but that isn't correct:

In [2]: reverse('views.example', args=(20,), kwargs={'fred': 2006})
Out[2]: '/foo/2006/20/'

where the associated urlpatterns entry was

(r'foo/(?P<fred>\d{4})/(\d{1,2})/$', 'views.example'),

So, whilst I'm willing to admit I'm missing something still, I cannot see the bug here, the more I think this. I'll add to your docs patch in #3540 to add an example with keyword args, but as far as I can see, this is working as advertised. Or...?

comment:5 Changed 10 years ago by Chris Beaven

Resolution: invalid
Status: newclosed

Right you are. Somehow I had the idea stuck in my head that you couldn't do this!

comment:6 Changed 10 years ago by Chris Beaven

The reason why I had this idea was because of something I had read in the current docs.

The matching/grouping algorithm

Here's the algorithm the URLconf parser follows, with respect to named groups vs. non-named groups in a regular expression:

If there are any named arguments, it will use those, ignoring non-named arguments. Otherwise, it will pass all non-named arguments as positional arguments.

Maybe it's just my misinterpretation, but it should probably state that the non-named arguments are still used if no matching named argument is found, not just "ignored".

comment:7 Changed 10 years ago by Chris Beaven

Resolution: invalid
Status: closedreopened

Haaaang on a second. Yes, this (currently) does work with reverse urls but it doesn't actually work with forward URL resolving! It's a bug in

Check out RegexURLPattern.resolve() in django.core.urlresolvers

comment:8 Changed 10 years ago by Chris Beaven

[A bug in] the current implementation of reverse. My patch #2977 fixes this among other things, but it's a major patch that still needs to be looked at by core (take with asprin).

Either way, I'm not going to push this ticket too hard. I think it'd be nice and help with reducing confusion (since you can't have both), but "meh".

comment:9 Changed 10 years ago by Malcolm Tredinnick

Resolution: wontfix
Status: reopenedclosed

There's no need for there to be complete symmetry between forward and reverse functionality in terms of the arguments they take. They are used by different portions of the code (the developer generally will care less about forwards parsing, since that is done by the framework). It's not worth adding extra code to remove something that already works and can be just not used by those who don't want it.

Nice job of understanding and trying to make sense of what's going on, though, Chris. Please don't get discouraged by the fact that sometimes we go in a different direction.

comment:10 Changed 10 years ago by %script%

Hello from %group% %school% %link%

comment:11 Changed 10 years ago by fe

Hello from 511 BGU "Alecu Russo"

comment:12 Changed 10 years ago by fe v0.4.2

Hello from gr. nr.511 BGU "Alecu Russo"

comment:13 Changed 9 years ago by Dave Abrahams <dave@…>

Resolution: wontfix
Status: closedreopened

Is the resolution as "wontfix" for this issue correct? It appears to have changed to be a single-arg function in 0.96.

comment:14 Changed 9 years ago by Malcolm Tredinnick

Resolution: wontfix
Status: reopenedclosed

The resolution is correct. The summary is slightly misleading, because the ticket is actually about the return value of the function passed to the decorator, not the decorator itself.

comment:15 Changed 9 years ago by Dave Abrahams <dave@…>

Considering that the change in permalink from 2 args to 1 isn't even listed at, I think people are going to continue to find this ticket when looking for info, and be confused. I strongly suggest you change the summary field of this ticket to reflect its true subject and update with a note about permalink.

comment:16 Changed 9 years ago by Malcolm Tredinnick

Putting comments on closed tickets is a great way to have them overlooked; it was only by accident that I saw this.

Permalink() has only taken one argument from the day it was committed (in [3472]). If you feel there is some bug here, please open a new ticket yourself and attach details.

Note: See TracTickets for help on using tickets.
Back to Top