Opened 13 years ago

Closed 9 years ago

#15180 closed New feature (duplicate)

reverse doesn't check default_args

Reported by: olaf Owned by: nobody
Component: Core (URLs) Version: 1.3-beta
Severity: Normal Keywords: resolve default_args
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Lets say we have the following urls:

    (r'^photos/?$', 'views.photo_index', {'lang':'en'}, "photo_index"),
    (r'^fotos/?$', 'views.photo_index', {'lang':'de'}, "photo_index"),
    (r'^photo/(?P<photo_id>[\d]+)$', 'views.photo', {"lang":"en"} ,"photo"),
    (r'^foto/(?P<photo_id>[\d]+)$', 'views.photo', {"lang":"de"} ,"photo"),

When I want to get the specific URL for a German visitor I wan't to just to get the reverse for photo an add a keyword argument {'lang':'de'}. It is very much writing to write for each language a different wrapper function. You also would need to write a bunch of if-else statements to select the correct one.

I saw that also some else expected this and asked a question at stackoverflow about this, but he got only workaround as answers. I added a few lines to correct behavior, and I hope this is useful for someone. I am not sure, if I broke something so it would be nice someone else could test this code also.

Attachments (3)

urlresolvers.diff (1.9 KB ) - added by olaf 13 years ago.
urlresolvers-v2.diff (3.4 KB ) - added by olaf 13 years ago.
urlresolvers-v2.2.diff (3.4 KB ) - added by olaf 13 years ago.

Download all attachments as: .zip

Change History (12)

by olaf, 13 years ago

Attachment: urlresolvers.diff added

comment:1 by Russell Keith-Magee, 13 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Russell Keith-Magee, 13 years ago

I have a background suspicion that this will cause all sorts of havoc with existing URL lookups -- for example, existing views with default arguments that *aren't* using the default arguments to differentiate between views.

However, since the patch doesn't have any tests, I can't verify this quickly.

Marking this as a qualified accepted, the qualification being that the implementation *must* be backwards compatible with existing uses, and be tested as such. If that can't be achieved, then we'll need to close wontfix.

comment:3 by olaf, 13 years ago

Ok, I changed my patch a little bit. I added a new optional parameter to the reverse function. If check_default_args is not set(=None), reverse tries to find a match the standard way, if it will not succeed it tries one time again including the default_args. If it is set to False, it behaves just like the current reverse function. If it is set to true, it will check them from the beginning.

I would like to write those test cases, but I don't know how to use different url configs. Could you point me to an Howto or give me a small example, and I will write them.

Thanks

by olaf, 13 years ago

Attachment: urlresolvers-v2.diff added

by olaf, 13 years ago

Attachment: urlresolvers-v2.2.diff added

comment:4 by Julien Phalip, 13 years ago

milestone: 1.3

This can't make it into 1.3 unless comprehensive regression tests are provided. Olaf, if you're interested in writing some tests, take a look at the guide available at http://docs.djangoproject.com/en/dev/topics/testing/
Don't hesitate to ask questions on the django-users list too if need be.

comment:5 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: New feature

comment:6 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 by Aymeric Augustin, 11 years ago

Component: Core (Other)Core (URLs)

comment:9 by Bas Peschier, 9 years ago

Resolution: duplicate
Status: newclosed

This was fixed in #13154

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