Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3683 closed (wontfix)

permalink decorator takes redundant arguments

Reported by: SmileyChris Owned by: adrian
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:

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

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by SmileyChris

Changed 8 years ago by SmileyChris

Uh, I meant this one

comment:1 Changed 8 years ago by SmileyChris

  • 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 8 years ago by mtredinnick

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 8 years ago by SmileyChris

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 8 years ago by mtredinnick

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 8 years ago by SmileyChris

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

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

comment:6 Changed 8 years ago by SmileyChris

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 8 years ago by SmileyChris

  • Resolution invalid deleted
  • Status changed from closed to reopened

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 8 years ago by SmileyChris

[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 8 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from reopened to closed

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

Hello from %group% %school% %link%

comment:11 Changed 8 years ago by fe

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

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

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

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

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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 8 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from reopened to closed

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

comment:16 Changed 8 years ago by mtredinnick

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