Opened 10 years ago

Closed 10 years ago

#24476 closed Cleanup/optimization (fixed)

Allow using set_script_prefix as a contextmanager

Reported by: Tim Graham Owned by: Bas Peschier
Component: Core (URLs) Version: dev
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: no

Description

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/')

Change History (9)

comment:1 by Bas Peschier, 10 years ago

Owner: changed from nobody to Bas Peschier
Status: newassigned

comment:2 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Bas Peschier, 10 years ago

Combining a contextmanager while allowing a normal function call turned out to be less trivial than thought. Changed it into a test util.

comment:4 by Baptiste Mispelon, 10 years ago

Something like this should work:

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

comment:5 by Bas Peschier, 10 years ago

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.

comment:6 by Baptiste Mispelon, 10 years ago

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 :)

comment:7 by Bas Peschier, 10 years ago

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.

comment:8 by Tim Graham, 10 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

Don't forget to check "Has patch" so the ticket appears in the review queue.

comment:9 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 0339844b:

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.

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