[Django] #24476: Allow using set_script_prefix as a contextmanager
Django
------------------------------------------------+------------------------
Reporter: timgraham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
It would be useful if `set_script_prefix()` could be used as a
contextmanager in addition to as a regular function. That would allow
replacing this redundant pattern in tests:
{{{
set_script_prefix('/beverages/')
try:
self.assertEqual(pf.get_absolute_url(), '/beverages/tea/')
finally:
clear_script_prefix()
}}}
with:
{{{
with set_script_prefix('/beverages/'):
self.assertEqual(pf.get_absolute_url(), '/beverages/tea/')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24476>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Component: Core (URLs) | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bpeschier):
* owner: nobody => bpeschier
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:1>
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:2>
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bpeschier):
Combining a contextmanager while allowing a normal function call turned
out to be less trivial than thought. Changed it into a test util.
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:3>
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bmispelon):
Something like this should work:
{{{#!python
class set_script_prefix(object):
"""
Sets the script prefix for the current thread.
Can be called directly or used as a context manager.
"""
def __init__(self, prefix):
if not prefix.endswith('/'):
prefix += '/'
self._old_prefix = get_script_prefix()
self._prefix = prefix
self._set_prefix(prefix)
def __enter__(self):
return self._prefix
def __exit__(self, exc_type, exc_val, exc_tb):
self._set_prefix(self._old_prefix)
def _set_prefix(self, prefix):
_prefixes.value = prefix
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:4>
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bpeschier):
Ah, yes. Combining context manager + call is doable, but not also with a
decorator.
I like the current implementation because it leaves the original code
simple. It only defines a helper for the test use case.
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:5>
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bmispelon):
You can probably use `contextlib.ContextDecorator` (backported in
`django.utils.decorators`) to make it into a decorator as well.
I don't have a particular opinion as to which approach is better though, I
just liked the challenge of trying to make it work :)
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:6>
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bpeschier):
It is! :-)
The PR uses ContextDecorator, but since a decorator gets called before you
do anything, it would set the script prefix while importing the module if
you put the meat in {{{__init__}}} which is required to make a direct call
work.
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:7>
Django
Reporter: timgraham | Owner: bpeschier
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
Don't forget to check "Has patch" so the ticket appears in the review
queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:8>
Django
Reporter: timgraham | Owner: bpeschier
Cleanup/optimization |
Component: Core (URLs) | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0339844b70895d6162b4595ae615e6edf843c6cd" 0339844b]:
{{{
#!CommitTicketReference repository=""
revision="0339844b70895d6162b4595ae615e6edf843c6cd"
Fixed #24476 -- Added context manager/decorator for overriding script
prefix.
Tests were using an undocumented keyword argument for easily overriding
script prefix while reversing. This is now changed into a test utility
which can be used as decorator or context manager.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24476#comment:9>