Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#3683 closed (wontfix)

permalink decorator takes redundant arguments

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

Description

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 17 years ago.
better_permalink.2.patch (2.8 KB ) - added by Chris Beaven 17 years ago.
Uh, I meant this one

Download all attachments as: .zip

Change History (18)

by Chris Beaven, 17 years ago

Attachment: better_permalink.patch added

by Chris Beaven, 17 years ago

Attachment: better_permalink.2.patch added

Uh, I meant this one

comment:1 by Chris Beaven, 17 years ago

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

comment:2 by Malcolm Tredinnick, 17 years ago

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 by Chris Beaven, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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 by Chris Beaven, 17 years ago

Resolution: invalid
Status: newclosed

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

comment:6 by Chris Beaven, 17 years ago

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 by Chris Beaven, 17 years ago

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 by Chris Beaven, 17 years ago

[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 by Malcolm Tredinnick, 17 years ago

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 by %script%, 17 years ago

Hello from %group% %school% %link%

comment:11 by fe, 17 years ago

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

comment:12 by fe v0.4.2, 17 years ago

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

comment:13 by Dave Abrahams <dave@…>, 17 years ago

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 by Malcolm Tredinnick, 17 years ago

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 by Dave Abrahams <dave@…>, 17 years ago

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.

comment:16 by Malcolm Tredinnick, 17 years ago

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