Opened 7 years ago

Closed 6 years ago

#13154 closed Bug (fixed)

Make reverse() match the behavior of resolve()

Reported by: Patryk Zawadzki Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

Consider the following urls.py:

urlpatterns = patterns('',
    (r'^faq/$', 'django.views.generic.simple.direct_to_template', {'template': 'faq.html'}, 'faq'),
    (r'^tos/$', 'django.views.generic.simple.direct_to_template', {'template': 'tos.html'}, 'tos'),
)

When passed to resolve(), /faq/ will yield:

>>> resolve('/faq/')
(<function direct_to_template at 0x8b2679c>, (), {'template': 'faq.html'})

However this will raise an exception:

>>> reverse('django.views.generic.simple.direct_to_template', kwargs={'template': 'faq.html'})
NoReverseMatch: Reverse for 'django.views.generic.simple.direct_to_template' with arguments '()' and keyword arguments '{'template': 'faq.html'}' not found.

The attached patch makes reverse() behave more intuitively and return a match when both the key and value match the default kwargs.

>>> reverse('django.views.generic.simple.direct_to_template', kwargs={'template': 'faq.html'})
'/faq/'

Attachments (3)

django-smarter-reverse.patch (3.1 KB) - added by Patryk Zawadzki 7 years ago.
Proposed patch
django-smarter-reverse-tests.patch (2.7 KB) - added by Patryk Zawadzki 7 years ago.
Tests, assuming that the mentioned URL can be fixed
django-smarter-reverse.2.patch (6.0 KB) - added by Patryk Zawadzki 6 years ago.
Updated patch containing both the change and the tests

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by Patryk Zawadzki

Proposed patch

comment:1 Changed 7 years ago by Russell Keith-Magee

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 years ago by Patryk Zawadzki

I'd provide a test but there is a line in tests/regressiontests/urlpatterns_reverse/urls.py that breaks it:

    url(r'^hardcoded/$', 'hardcoded/', empty_view, name="hardcoded"),

I am not quite sure how to interpret that line but it assigns the view to the kwargs. What should I do about it?

comment:3 Changed 7 years ago by Russell Keith-Magee

@patrys -- I can't think of any reason that you would need to pass the view as the kwargs, and if you remove the second 'hardcoded/' argument, the test suite still passes. I'm going to chalk this one up to a bug that didn't get caught because the existing reverse logic didn't walk that particular code path, and there is nothing in the urlpatterns_reverse tests that actually tries to use the view.

Feel free to correct that test as part of your patch for this ticket.

Changed 7 years ago by Patryk Zawadzki

Tests, assuming that the mentioned URL can be fixed

comment:4 Changed 7 years ago by Patryk Zawadzki

Needs tests: unset

comment:5 Changed 7 years ago by Patryk Zawadzki

Forgot that Trac doesn't send upload notifications. I've attached the tests and fixed the hardcoded/ one as suggested.

comment:6 Changed 6 years ago by Michał Sałaban

Can we have this patch applied to 1.3? This change is backwards-compatible and very useful too.

comment:7 Changed 6 years ago by Luke Plant

Type: Bug

comment:8 Changed 6 years ago by Luke Plant

Severity: Normal

comment:9 Changed 6 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set

django-smarter-reverse-tests.patch fails to apply cleanly on to trunk

Changed 6 years ago by Patryk Zawadzki

Updated patch containing both the change and the tests

comment:10 Changed 6 years ago by Patryk Zawadzki

Patch needs improvement: unset

Updated the patch to apply to current SVN trunk.

comment:11 Changed 6 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

comment:12 Changed 6 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16177]:

Fixed #13154 -- Fixed the URL resolver's reverse() to match the behavior of its resolve() with regard to the default kwargs. Many thanks to patrys.

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