Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contextlib.ContextDecorator #53356

Closed
voidspace opened this issue Jun 28, 2010 · 20 comments
Closed

contextlib.ContextDecorator #53356

voidspace opened this issue Jun 28, 2010 · 20 comments
Assignees
Labels
docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error

Comments

@voidspace
Copy link
Contributor

voidspace commented Jun 28, 2010

BPO 9110
Nosy @birkenfeld, @rhettinger, @ncoghlan, @jackdied, @voidspace
Files
  • contextdecorator.patch
  • docs.patch
  • contextdecorator.2.patch
  • contextlib_docs.patch: Doc extension.
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/voidspace'
    closed_at = <Date 2010-07-18.21:12:12.066>
    created_at = <Date 2010-06-28.22:58:17.341>
    labels = ['type-bug', 'docs']
    title = 'contextlib.ContextDecorator'
    updated_at = <Date 2010-07-18.21:12:12.065>
    user = 'https://github.com/voidspace'

    bugs.python.org fields:

    activity = <Date 2010-07-18.21:12:12.065>
    actor = 'ncoghlan'
    assignee = 'michael.foord'
    closed = True
    closed_date = <Date 2010-07-18.21:12:12.066>
    closer = 'ncoghlan'
    components = ['Documentation']
    creation = <Date 2010-06-28.22:58:17.341>
    creator = 'michael.foord'
    dependencies = []
    files = ['17795', '17796', '17808', '18049']
    hgrepos = []
    issue_num = 9110
    keywords = ['patch']
    message_count = 20.0
    messages = ['108877', '108906', '108907', '108911', '108975', '108976', '108977', '109811', '109812', '109813', '109814', '109815', '109828', '109833', '109834', '109835', '109897', '110358', '110643', '110645']
    nosy_count = 6.0
    nosy_names = ['georg.brandl', 'rhettinger', 'ncoghlan', 'jackdied', 'michael.foord', 'docs@python']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue9110'
    versions = ['Python 3.2']

    @voidspace
    Copy link
    Contributor Author

    voidspace commented Jun 28, 2010

    Patch to add a ContextDecorator class to contextlib. This allows context managers that inherit from ContextDecorator (including using it as a mixin) to be used as decorators as well as context managers.

    Context managers inheriting from ContextDecorator still have to implement __enter__ and __exit__ as normal. As the decorator behaviour is implemented using a with statement __exit__ retains its optional exception handling even when used as a decorator.

    In addition contextlib.GeneratorContextManager, used to implement contextlib.contextmanager, inherits from ContextDecorator. Context managers created with contextlib.contextmanager can therefore be used as decorators too.

    @voidspace voidspace added the stdlib Python modules in the Lib dir label Jun 28, 2010
    @voidspace voidspace self-assigned this Jun 28, 2010
    @voidspace voidspace added the type-bug An unexpected behavior, bug, or error label Jun 28, 2010
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 29, 2010

    Looks pretty good to me, but you may want to doublecheck some of your test criteria.

    Firstly, the two "*_with_exception" checks don't look quite right to me. I would have expected to see something like the following for both of them:

      self.assertIsNotNone(context.exc)
      self.assertIs(context.exc[0], NameError).

    Secondly, I can't even begin to guess what the method decoration test is currently trying to show. Why 3 instantiations? Why check the instance assignments rather than the context manager behaviour? Either I'm completely missing something, or this currently isn't testing what you meant to test :)

    Finally, you may as well include a second typo test to cover the __enter__ misspelling case.

    @voidspace
    Copy link
    Contributor Author

    voidspace commented Jun 29, 2010

    Hey Nick,

    The tests are pretty much just copied from the previous version, so they aren't all appropriate. In fact I think that the first two tests (and even the typo tests) can just go as they really just test Python semantics and not the ContextDecorator itself.

    The method tests are testing that the arguments (including keyword arguments) are properly passed through to the decorated function. I started to write a comment to that effect as I realised it wasn't obvious, but forgot to finish the comment... Separate instantiations just mean we don't need to worry about previous values.

    (I did them as methods as I was *starting* to write a test for general descriptor decorating - but the general case for that is very hard to solve so people just have to use their decorators in the right order instead. Nonetheless it is nice to have a test that decorates a method as well as the other function decorating tests.)

    Your improvement to the exceptions tests are good.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 29, 2010

    On Wed, Jun 30, 2010 at 12:13 AM, Michael Foord <report@bugs.python.org> wrote:

    Michael Foord <michael@voidspace.org.uk> added the comment:

    Hey Nick,

    The tests are pretty much just copied from the previous version, so they aren't all appropriate. In fact I think that the first two tests (and even the typo tests) can just go as they really just test Python semantics and not the ContextDecorator itself.

    Ah, yep, I can see how they would have been more important with the
    old 2.4 compatible implementation. However, it's still useful to keep
    them as a sanity check that the test context manager isn't doing
    anything silly (i.e. if they pass and the decorator tests fail,
    ContextDecorator is obviously at fault, whereas without these first
    two tests the bug could be in the test context manager).

    The method tests are testing that the arguments (including keyword arguments) are properly passed through to the decorated function. I started to write a comment to that effect as I realised it wasn't obvious, but forgot to finish the comment... Separate instantiations just mean we don't need to worry about previous values.

    (I did them as methods as I was *starting* to write a test for general descriptor decorating - but the general case for that is very hard to solve so people just have to use their decorators in the right order instead. Nonetheless it is nice to have a test that decorates a method as well as the other function decorating tests.)

    That makes sense, but a comment to that effect would be *very* helpful :)

    Comments on the docs patch (I neglected to look at that initially):

    For the @contextmanager updates, I would move the description of the
    effect into an extra paragraph in the main body of the description,
    then just use the versionadded tag to when the change was made.

    For the ContextDecorator description itself, you may want to write the
    last part of the first example (i.e. just the with statement) in a
    doctest layout so you can show the expected output.

    @voidspace
    Copy link
    Contributor Author

    voidspace commented Jun 30, 2010

    New patch uploaded with (I think) all suggested changes made. I left some text in the .. versionchanged:: tag for contextmanager as I think this is normal.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 30, 2010

    On Wed, Jun 30, 2010 at 9:51 PM, Michael Foord <report@bugs.python.org> wrote:

    I left some text in the .. versionchanged:: tag for contextmanager as I think this is normal.

    Yeah, what you did was what I meant (I just didn't say it very well)

    The example use in the docs should call function() rather than invoke
    the context manager explicitly.

    Otherwise looks good, so I'd say go ahead and check it in :)

    @voidspace
    Copy link
    Contributor Author

    voidspace commented Jun 30, 2010

    Committed revision 82394.

    I left examples of using ContextDecorator with a decorated *and* in a with statement in the documentation. Feel free to trim the with statement example if you feel it is redundant.

    @jackdied
    Copy link
    Contributor

    jackdied commented Jul 10, 2010

    I like it, but I think it would help to give it the same interface as contextlib.contextmanager (the single function, single yield). Like your mock library 'patch' both function decorators and context managers have an interface that reads like "do this before the real work," "do the real work," and then "do this after the real work" pattern. The fact that the examples and test cases all require an almost empty class feels heavy to me.

    @voidspace
    Copy link
    Contributor Author

    voidspace commented Jul 10, 2010

    Hey Jack. The point of ContextDecorator is that when you are implementing a context manager, which you will usually do as a class anyway, you can just inherit from it and get the decorator functionality for free.

    If you like the contextmanager style of creating your apis then you can just use contextmanager - which now uses ContextDecorator *anyway*. (i.e. all uses of contextlib.contextmanager can now be used as decorators as well as context managers.)

    @jackdied
    Copy link
    Contributor

    jackdied commented Jul 10, 2010

    Hey Frood, I'll take another look at it tomorrow when I am less addled. But as to context managers that are actual classes - I've not written a single one; they are always generator functions with a simple try/yield/except/finally in the body. After all state-is-state, and writing an __init__, __enter__, and __exit__ is just extra boilerplate for my common uses.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 10, 2010

    I can shake the feeling that this is going to end-up being rarely used cruft that causes more confusion than its worth. The use case and motivation isn't clear (why do context managers need to be both context managers and decorators)?

    Has this decorator/contextmanager mix been used "in the wild" and shown any degree of popularity? AFAICT, it doesn't seem to have come in newsgroup discussions, feature requests, utility modules, or a published recipe.

    Do we have any strong examples of code that is improved by the use of a decorating context manager?

    One issue for me is that my understanding of decorators (as function/classs wrappers) and of context managers (deeply associated with the with-statement) have no overlap. ISTM, that code using both in the same object would be too clever by far, making it difficult to explain and difficult to debug.

    Also, Jack's critiques seem valid to me -- too much machinery for too little benefit.

    @jackdied
    Copy link
    Contributor

    jackdied commented Jul 10, 2010

    Raymond,

    Short version: This isn't theoretical because I use context managers and function decorators interchangeably and constantly.

    Long Version: Function decorators and context managers have very similar use cases. They both go something like:

    1. add optional extra state
    2. execute the original function (decorator) or block (context manager)
    3. add optional extra exception handling or do something special based on the extra state.

    Frood's mock library does this in a very sane way. ex/
    @mock.patch(sys, 'stdio', someStringIOInstance)
    def test_blah(self): pass

    # this test uses context instead of decorators
    def test_blaise(self):
      # test setup here
      with @mock.patch(sys, 'stdin', someStringIOInstance):
        dummy = 'something particular to this setup'
      # more tests here

    So the use isn't theoretical [at a minimum he's doing it and I'm doing it], now we're just talking about what is the most obvious interface.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 10, 2010

    Jack, thanks for the explanation. Glad it seems sensible to more than just one person. To my eyes, it looks like a little too much magic and I would be more comfortable if this idea and its variants had been explored more thoroughly by the community before it got injected into the standard library.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 10, 2010

    The idea of building this into contextlib actually came out on off-list discussion between Michael and I. To quote the original suggestion he sent to me:

    """What do you think about adding ContextDecorator to contextlib for Python 3.2?

    http://pypi.python.org/pypi/contextdecorator
    

    It's a simple recipe but useful nonetheless (and Barry Warsaw at least is very enthusiastic about it ;-).

    ContextDecorator allows you to create APIs that behave as both context managers and as decorators. It also provides the optional exception handling capability of __exit__ for decorators. This isn't an uncommon pattern, used in libraries like mock, py.test and django, and it is at least slightly fiddly to get right."""

    I agree it is good to have that additional motivation (and the reference to previous work) here in the tracker issue rather than squirreled away in a couple of private email archives.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 10, 2010

    One thing that Jack's confusion above does suggest to me is that we should mention in the *ContextDecorator* documentation that it is automatically applied to the context managers created when you use @contextmanager. A lot of people familiar with contextmanager are just going to read the docs for the new toy, so may miss the fact that we have added __call__ support to GeneratorContextManager.

    As far as use cases go, this change is just syntactic sugar for any construct of the following form:

      def f():
        with cm():
          # Do stuff

    ContextDecorator lets you instead write:

      @cm
      def f():
        # Do stuff

    It makes it clear that the CM applies to the whole function, rather than just a piece of it (and saving an indentation level is nice, too).

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 10, 2010

    this change is just syntactic sugar for
    any construct of the following form:

    def f():
    with cm():
    # Do stuff

    ContextDecorator lets you instead write:

    @cm
    def f():

    Do stuff

    Nicely expressed. This ought to go directly into the documentation.

    @voidspace
    Copy link
    Contributor Author

    voidspace commented Jul 10, 2010

    I agree that mentioning contextmanager in the ContextDecorator docs is a good idea - plus adding something like Nick's example. I'll do that.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 15, 2010

    Bumping back to open until the documentation is tweaked.

    @ncoghlan ncoghlan added docs Documentation in the Doc dir and removed stdlib Python modules in the Lib dir labels Jul 15, 2010
    @ncoghlan ncoghlan reopened this Jul 15, 2010
    @voidspace
    Copy link
    Contributor Author

    voidspace commented Jul 18, 2010

    I've uploaded a new patch with additions to the docs. Let me know if it is ok or if you have any suggested amendments.

    @birkenfeld
    Copy link
    Member

    birkenfeld commented Jul 18, 2010

    Applied, with small changes, in r82951.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants