Opened 5 years ago

Closed 4 years ago

#13154 closed Bug (fixed)

Make reverse() match the behavior of resolve()

Reported by: patrys 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 patrys 5 years ago.
Proposed patch
django-smarter-reverse-tests.patch (2.7 KB) - added by patrys 5 years ago.
Tests, assuming that the mentioned URL can be fixed
django-smarter-reverse.2.patch (6.0 KB) - added by patrys 4 years ago.
Updated patch containing both the change and the tests

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by patrys

Proposed patch

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by patrys

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 5 years ago by russellm

@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 5 years ago by patrys

Tests, assuming that the mentioned URL can be fixed

comment:4 Changed 5 years ago by patrys

  • Needs tests unset

comment:5 Changed 5 years ago by patrys

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

comment:6 Changed 5 years ago by emes

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

comment:7 Changed 4 years ago by lukeplant

  • Type set to Bug

comment:8 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:9 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

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

Changed 4 years ago by patrys

Updated patch containing both the change and the tests

comment:10 Changed 4 years ago by patrys

  • Patch needs improvement unset

Updated the patch to apply to current SVN trunk.

comment:11 Changed 4 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 4 years ago by jezdez

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

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