Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#9310 closed (fixed)

404 debug pages should show the name of the tried urlpattern if one exists.

Reported by: floguy Owned by: Tobias McNulty
Component: Core (Other) Version: 1.0
Severity: Keywords:
Cc: Tobias McNulty, Cyberj Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Sometimes it's useful to know what the names of the url patterns were that were being tried. I wrote a small little patch to show that information.

Attachments (3)

named_404.diff (1.7 KB) - added by floguy 8 years ago.
Patch to enable this functionality.
named_404_with_test_2.diff (7.0 KB) - added by Cyberj 7 years ago.
Patch improved to fit with rev12403
named_404_with_test.diff (7.0 KB) - added by Tobias McNulty 6 years ago.
REALLY updated the patch to latest trunk

Download all attachments as: .zip

Change History (26)

Changed 8 years ago by floguy

Attachment: named_404.diff added

Patch to enable this functionality.

comment:1 Changed 8 years ago by Malcolm Tredinnick

Needs documentation: unset
Needs tests: set
Patch needs improvement: set

You seem to have left off the part of the patch that contained the tests.

I'm fairly sure that when you write those tests you're going to find that line 183 is a problem. The existing code adds 't' to a string. Your new code is adding the same 't' to a list. That looks like a problem.

comment:2 Changed 8 years ago by Malcolm Tredinnick

Also the new code on line 183 is using a generator expression, which isn't valid in version of python before 2.5. It needs to remain as a list comprehension.

comment:3 Changed 8 years ago by floguy

You're right that it's missing tests. I'll work on that just as soon as I get some time in the next few days.

However I think that you're mistaken that line 183 is a problem. It's a recursive call, so by changing the type of the first argument, we're changing the type of all of the t instances. I can't prove it, because I don't have tests (thus reinforcing your first point), but it does work for me.

Good point about the generator expression, though. I've been spending too much time in py2.5 land at work, that I forget what's valid 2.3 code :)

comment:4 Changed 8 years ago by Jacob

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:5 Changed 7 years ago by Jacob

milestone: 1.11.2

comment:6 Changed 7 years ago by Tobias McNulty

Owner: changed from nobody to Tobias McNulty
Status: newassigned

working on this at djangocon sprints with adrian nye

comment:7 Changed 7 years ago by Tobias McNulty

The code needs to make a list instead of a string so the template can iterate through the different resolvers and patterns (and hence have access to the final pattern's name). The template has been updated to work with the new format and I couldn't find anything else that depends on Resolver404's 'tried' arg.

I updated Eric's patch to apply to the latest trunk, converted the generator expression into a list comprehension, and created a test for the new feature.

Let me know what you think.

comment:8 Changed 7 years ago by Tobias McNulty

Owner: changed from Tobias McNulty to nobody
Status: assignednew

bumping back to nobody for review and testing by someone else

comment:9 Changed 7 years ago by Tobias McNulty

Needs tests: unset

comment:10 Changed 7 years ago by Tobias McNulty

Cc: Tobias McNulty added

comment:11 Changed 7 years ago by Tobias McNulty

Owner: changed from nobody to Tobias McNulty
Status: newassigned

looks like patch needs updating

Changed 7 years ago by Cyberj

Attachment: named_404_with_test_2.diff added

Patch improved to fit with rev12403

comment:12 Changed 7 years ago by Cyberj

Patch needs improvement: unset

Tests are Ok for this patch, waiting for second review.

comment:13 Changed 7 years ago by Cyberj

Cc: Cyberj added

comment:14 Changed 7 years ago by Tobias McNulty

Applies against trunk and all tests pass

comment:15 Changed 7 years ago by James Bennett

milestone: 1.2

Though it's a minor feature request, this is still a feature request and thus not in scope for 1.2.

comment:16 Changed 6 years ago by Tobias McNulty

milestone: 1.3

patch updated to apply to trunk; setting milestone to 1.3

comment:17 Changed 6 years ago by Tobias McNulty

Triage Stage: AcceptedReady for checkin

marking RFC per mtredinnick

comment:18 Changed 6 years ago by Malcolm Tredinnick

I hate to be picky, but this patch doesn't apply to trunk! Things like urlresolvers.py have changed. Can you check your checkout has r13678 included, since that was the last time urlresolvers.py was changed.

comment:19 Changed 6 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Changed 6 years ago by Tobias McNulty

Attachment: named_404_with_test.diff added

REALLY updated the patch to latest trunk

comment:20 Changed 6 years ago by Tobias McNulty

apparently I managed to clone the bitbucket.org/django/django repo this morning before it had been officially released, so I had a version from a couple weeks ago. I pulled the latest, resolved the conflict, and attached the updated patch.

comment:21 Changed 6 years ago by Tobias McNulty

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:22 Changed 6 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(In [13769]) Debug 404 page now displays names of URL patterns, if they exist.

Thanks to Tobias McNulty for the patch. Fixed #9310.

comment:23 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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