permalink decorator takes redundant arguments

[Chris Beaven]

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.

[Chris Beaven]

Uh, I meant this one

[Chris Beaven]

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

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

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

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

[Chris Beaven]

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

[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".

[Chris Beaven]

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

[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".

[Malcolm Tredinnick]

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.

[%script%]

Hello from %group% %school% %link%

[fe]

Hello from 511 BGU "Alecu Russo" ​http://www.usb.md/ro/

[fe v0.4.2]

Hello from gr. nr.511 BGU "Alecu Russo" ​http://www.usb.md/ro/

[Dave Abrahams <dave@…>]

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

[Malcolm Tredinnick]

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.

[Dave Abrahams <dave@…>]

Considering that the change in permalink from 2 args to 1 isn't even listed at ​http://code.djangoproject.com/wiki/BackwardsIncompatibleChanges, 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 ​http://code.djangoproject.com/wiki/BackwardsIncompatibleChanges with a note about permalink.

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