classification
Title: contextlib.ContextDecorator
Type: behavior Stage: resolved
Components: Documentation Versions: Python 3.2
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: michael.foord Nosy List: docs@python, georg.brandl, jackdied, michael.foord, ncoghlan, rhettinger
Priority: normal Keywords: patch

Created on 2010-06-28 22:58 by michael.foord, last changed 2010-07-18 21:12 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
contextdecorator.patch michael.foord, 2010-06-28 22:58
docs.patch michael.foord, 2010-06-28 23:22
contextdecorator.2.patch michael.foord, 2010-06-30 11:51
contextlib_docs.patch michael.foord, 2010-07-18 13:06 Doc extension.
Messages (20)
msg108877 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-06-28 22:58
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.
msg108906 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-06-29 14:06
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.
msg108907 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-06-29 14:13
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.
msg108911 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-06-29 14:29
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.
msg108975 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-06-30 11:51
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.
msg108976 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-06-30 11:57
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 :)
msg108977 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-06-30 12:19
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.
msg109811 - (view) Author: Jack Diederich (jackdied) * (Python committer) Date: 2010-07-10 02:23
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.
msg109812 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-07-10 02:27
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.)
msg109813 - (view) Author: Jack Diederich (jackdied) * (Python committer) Date: 2010-07-10 02:40
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.
msg109814 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-07-10 03:02
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.
msg109815 - (view) Author: Jack Diederich (jackdied) * (Python committer) Date: 2010-07-10 03:39
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.
msg109828 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-07-10 07:20
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.
msg109833 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-07-10 07:59
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.
msg109834 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-07-10 08:16
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).
msg109835 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-07-10 09:00
> 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.
msg109897 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-07-10 17:20
I agree that mentioning contextmanager in the ContextDecorator docs is a good idea - plus adding something like Nick's example. I'll do that.
msg110358 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-07-15 11:44
Bumping back to open until the documentation is tweaked.
msg110643 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2010-07-18 13:06
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.
msg110645 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-07-18 13:43
Applied, with small changes, in r82951.
History
Date User Action Args
2010-07-18 21:12:12ncoghlansetstatus: open -> closed
stage: needs patch -> resolved
2010-07-18 13:43:36georg.brandlsetnosy: + georg.brandl
messages: + msg110645
2010-07-18 13:06:01michael.foordsetfiles: + contextlib_docs.patch

messages: + msg110643
2010-07-15 11:46:31eric.araujosetnosy: + docs@python

stage: resolved -> needs patch
2010-07-15 11:44:49ncoghlansetstatus: closed -> open

messages: + msg110358
components: + Documentation, - Library (Lib)
nosy: rhettinger, ncoghlan, jackdied, michael.foord
2010-07-10 17:20:17michael.foordsetmessages: + msg109897
2010-07-10 09:00:52rhettingersetmessages: + msg109835
2010-07-10 08:16:44ncoghlansetmessages: + msg109834
2010-07-10 07:59:58ncoghlansetmessages: + msg109833
2010-07-10 07:20:17rhettingersetmessages: + msg109828
2010-07-10 03:39:24jackdiedsetmessages: + msg109815
2010-07-10 03:02:30rhettingersetnosy: + rhettinger
messages: + msg109814
2010-07-10 02:40:18jackdiedsetmessages: + msg109813
2010-07-10 02:27:06michael.foordsetmessages: + msg109812
2010-07-10 02:23:41jackdiedsetnosy: + jackdied
messages: + msg109811
2010-06-30 12:19:13michael.foordsetstatus: open -> closed
resolution: accepted
messages: + msg108977

stage: patch review -> resolved
2010-06-30 11:57:37ncoghlansetmessages: + msg108976
2010-06-30 11:51:05michael.foordsetfiles: + contextdecorator.2.patch

messages: + msg108975
2010-06-29 14:29:09ncoghlansetmessages: + msg108911
2010-06-29 14:13:36michael.foordsetmessages: + msg108907
2010-06-29 14:06:40ncoghlansetmessages: + msg108906
2010-06-28 23:22:12michael.foordsetfiles: + docs.patch
2010-06-28 22:58:17michael.foordcreate