Opened 4 years ago
Closed 4 years ago
#32480 closed Cleanup/optimization (fixed)
Outdated docstring in permission_denied() and redundant comments in default error pages.
Reported by: | BeryCZ | Owned by: | BeryCZ |
---|---|---|---|
Component: | Error reporting | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Claude Paroz | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hello,
I pulled a request on github to fix some things in django/views/defaults.py .
I was writing custom views for 400/403/404/500 because I needed them to return TemplateResponse, so I looked into the default ones for inspiration and I found some confusing things there:
1) the permission_denied() returns
context={'exception': str(exception)}
while the page_not_found() uses a nicer
exception_repr = exception.__class__.__name__ # Try to get an "interesting" exception message, if any (and not the ugly # Resolver404 dictionary) try: message = exception.args[0] except (AttributeError, IndexError): pass else: if isinstance(message, str): exception_repr = message context = { 'request_path': quote(request.path), 'exception': exception_repr, }
I suppose someone just forgot to read the whole code when making some changes, so it might be better to use the same code to get the exception message...?
2) permission_denied() has in the function description:
Context: None
while it should be
Context: exception The message from the exception which triggered the 403 (if one was supplied), or the exception class name
3) There is twice this comment about csrf_token, before page_not_found() and permission_denied()
# This can be called when CsrfViewMiddleware.process_view has not run, # therefore need @requires_csrf_token in case the template needs # {% csrf_token %}.
But you actually have the decorator @requires_csrf_token in front of all the views, so it might kinda confuse people who read that.
Thanks for all the work,
with regards,
Bery
Change History (3)
comment:1 by , 4 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Error reporting |
Has patch: | set |
Owner: | changed from | to
Patch needs improvement: | set |
Status: | new → assigned |
Summary: | Standardization of the exception message in permission_denied() + fixing the comments in views.defaults → Outdated docstring in permission_denied() and redundant comments in default error pages. |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 4 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Replying to BeryCZ:
That's not true, it was discussed in the original ticket #24733, see comments. I don't see any reason to use the same special treatment for 403.
I agree with fixing comments and docstrings.
PR