Code

Opened 6 years ago

Closed 4 years ago

Last modified 3 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
Component: Core (Other) Version: 1.0
Severity: Keywords:
Cc: tobias, 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 6 years ago.
Patch to enable this functionality.
named_404_with_test_2.diff (7.0 KB) - added by Cyberj 4 years ago.
Patch improved to fit with rev12403
named_404_with_test.diff (7.0 KB) - added by tobias 4 years ago.
REALLY updated the patch to latest trunk

Download all attachments as: .zip

Change History (26)

Changed 6 years ago by floguy

Patch to enable this functionality.

comment:1 Changed 6 years ago by mtredinnick

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

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

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 5 years ago by jacob

  • milestone changed from 1.1 to 1.2

comment:6 Changed 5 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

working on this at djangocon sprints with adrian nye

comment:7 Changed 5 years ago by tobias

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

  • Owner changed from tobias to nobody
  • Status changed from assigned to new

bumping back to nobody for review and testing by someone else

comment:9 Changed 5 years ago by tobias

  • Needs tests unset

comment:10 Changed 5 years ago by tobias

  • Cc tobias added

comment:11 Changed 4 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

looks like patch needs updating

Changed 4 years ago by Cyberj

Patch improved to fit with rev12403

comment:12 Changed 4 years ago by Cyberj

  • Patch needs improvement unset

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

comment:13 Changed 4 years ago by Cyberj

  • Cc Cyberj added

comment:14 Changed 4 years ago by tobias

Applies against trunk and all tests pass

comment:15 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

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

comment:16 Changed 4 years ago by tobias

  • milestone set to 1.3

patch updated to apply to trunk; setting milestone to 1.3

comment:17 Changed 4 years ago by tobias

  • Triage Stage changed from Accepted to Ready for checkin

marking RFC per mtredinnick

comment:18 Changed 4 years ago by mtredinnick

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

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

Changed 4 years ago by tobias

REALLY updated the patch to latest trunk

comment:20 Changed 4 years ago by tobias

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 4 years ago by tobias

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:22 Changed 4 years ago by mtredinnick

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

(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 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.