Issue11647
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2011-03-23 02:04 by pitrou, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
issue_11647.diff | ysj.ray, 2011-03-29 08:33 | |||
issue_11647_2.diff | ysj.ray, 2011-03-30 13:19 | |||
issue11647_refresh_cm.diff | ncoghlan, 2012-05-19 12:52 | Patch to make the refresh_cm() method public | review |
Messages (21) | |||
---|---|---|---|
msg131838 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2011-03-23 02:04 | |
If you add the following test to text_contextlib: diff --git a/Lib/test/test_contextlib.py b/Lib/test/test_contextlib.py --- a/Lib/test/test_contextlib.py +++ b/Lib/test/test_contextlib.py @@ -363,6 +363,8 @@ class TestContextDecorator(unittest.Test state.append(x) test('something') self.assertEqual(state, [1, 'something', 999]) + test('something else') + self.assertEqual(state, [1, 'something', 999, 1, 'something else', 999]) # This is needed to make the test actually run under regrtest.py! You get the following error: ====================================================================== ERROR: test_contextmanager_as_decorator (test.test_contextlib.TestContextDecorator) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/antoine/cpython/32/Lib/contextlib.py", line 28, in __enter__ return next(self.gen) StopIteration During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/antoine/cpython/32/Lib/test/test_contextlib.py", line 366, in test_contextmanager_as_decorator test('something else') File "/home/antoine/cpython/32/Lib/contextlib.py", line 15, in inner with self: File "/home/antoine/cpython/32/Lib/contextlib.py", line 30, in __enter__ raise RuntimeError("generator didn't yield") RuntimeError: generator didn't yield Clearly there is a problem in the API or its implementation, as in a function decorated with a context manager can only be invoked once! This isn't good... |
|||
msg131851 - (view) | Author: Alyssa Coghlan (ncoghlan) * | Date: 2011-03-23 04:10 | |
On Wed, Mar 23, 2011 at 12:04 PM, Antoine Pitrou <report@bugs.python.org> wrote: > Clearly there is a problem in the API or its implementation, as in a function decorated with a context manager can only be invoked once! This isn't good... The problem stems from a fundamentally bad call on my part: making ContextGenerator inherit from ContextDecorator. I failed to account for the fact that in order to work correctly the latter requires "reusable" context managers that can be used more than once, but ContextGenerator objects are one-shots (i.e. once __enter__ has been called once, you can't use them again). We can either hack this to work by providing ContextDecorator with a way to get the underlying context manager to create a new copy of itself each time, or else revert to the 3.1 status quo and declare that context managers created via contextlib.contextmanager simply can't be used as decorators. I'm inclined to favour the latter option. |
|||
msg131931 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2011-03-23 22:18 | |
> We can either hack this to work by providing ContextDecorator with a > way to get the underlying context manager to create a new copy of > itself each time, or else revert to the 3.1 status quo and declare > that context managers created via contextlib.contextmanager simply > can't be used as decorators. I'm inclined to favour the latter option. I would vote for the former option :) |
|||
msg131944 - (view) | Author: Michael Foord (michael.foord) * | Date: 2011-03-23 23:39 | |
The former sounds like fixing the bug, the latter like removing functionality. So yes, I prefer the former... |
|||
msg131957 - (view) | Author: Alyssa Coghlan (ncoghlan) * | Date: 2011-03-24 05:07 | |
> The former sounds like fixing the bug, the latter like removing functionality. So yes, I prefer the former... The reason I'm reluctant is that it makes for a fundamental change in behaviour between the use of an @contextmanager definition as a context manager and as a decorator. The former would continue to be a one-shot operation, just like opening a file and implicitly closing it at the end. The latter (if fixed) would need to implicitly recreate the context manager on each call to the function, creating the illusion of the context manager being reusable. The difference in underlying behaviour bothers me a great deal, as does the effective creation of a "context decorator" protocol that is the moral equivalent of the implicit context manager __with__ method that received barely any interest when I posted it on python-ideas. Given that it is impossible for anyone to be using this feature at this stage (since we just added it and it doesn't actually work in practice), I would prefer to simply acknowledge that I screwed up in failing to completely think through the implications of adding it and ditch it completely. If someone really wants this functionality to work, then they can do it right and write the implicit context manager PEP. (Note that ContextDecorator itself is fine, as long as the context manager is reusable. It's the fact that ContextGenerator *isn't* reusable that causes problems). |
|||
msg132090 - (view) | Author: ysj.ray (ysj.ray) | Date: 2011-03-25 14:17 | |
Agreed with nick's idea, the implicitly recreation of the context managers would confuse users. How about removing the "generator must yield exactly one value" restriction of @contextlib.contextmanage? Then if I want a generator to be used as a common context manager, I can make it yield exactly one value, else further if I want the resulting context manager can be used as a decorator, (reusable), I can put the generator code in a "while True" loop and add a "yield" at the end of loop body, like this: @contextmanager def func(): while True: print('begin func') yield print('end func') yield :) |
|||
msg132094 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2011-03-25 14:38 | |
> Agreed with nick's idea, the implicitly recreation of the context > managers would confuse users. Uh, why would it? That's exactly what I expect the decorator to do, and I was astonished to discover that it *doesn't*. |
|||
msg132096 - (view) | Author: ysj.ray (ysj.ray) | Date: 2011-03-25 14:50 | |
> > Agreed with nick's idea, the implicitly recreation of the context > > managers would confuse users. > Uh, why would it? That's exactly what I expect the decorator to do, and > I was astonished to discover that it *doesn't*. Because there is no *OBVIOUS* code or sign which can illustrate that context manager changes from a one-shot to a reusable. When using in "with" statement, it's a one-shot, while using as a decorator, it becomes a reusable. I think the way using it doesn't provide enough reason for its behavior change. Is there somebody who may expected that the GeneratorContextManager IS a one-shot? |
|||
msg132100 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2011-03-25 15:24 | |
> Because there is no *OBVIOUS* code or sign which can illustrate that > context manager changes from a one-shot to a reusable. I'm talking about the decorator, not the context manager. Surely there is a way for the decorator to instantiate a new context manager each time the decorated function is called? I may miss something, but it doesn't sound like rocket science. |
|||
msg132189 - (view) | Author: Michael Foord (michael.foord) * | Date: 2011-03-26 00:15 | |
I'm strongly with Antoine on this. If you use context manager as a decorator you should be able to reuse it - and in general both generators and context managers can be reused and so I don't understand the phrase 'moral equivalent of the implicit context manager'. If having APIs that can be used as context managers *and* decorators is a useful thing (and usage in django, py.test and mock seems to show that it is) then this functionality should be provided. |
|||
msg132401 - (view) | Author: ysj.ray (ysj.ray) | Date: 2011-03-28 14:49 | |
Now I found that I am wrong and I feel OK with make the context manager reusable when used as a decorator. I missed one thing: the reusable behavior is regarded as in the decorated function but not in the decorator context manager. :) |
|||
msg132471 - (view) | Author: ysj.ray (ysj.ray) | Date: 2011-03-29 08:33 | |
Here I worked out a patch to make the function decorated by context manager returned by @contextmanager reusable, by storing original function object and argments(args, kwds objects) in _GeneratorContextManager and construct a generator when needed(in self.__enter__ while used as a context manager and self.__call__ while used as a decorator). It still inherit ContextDecorator to keep class inheritation complete and the __call__ method is override. |
|||
msg132475 - (view) | Author: Alyssa Coghlan (ncoghlan) * | Date: 2011-03-29 10:30 | |
Part of the problem is that there was a specific reason we didn't go with lazy initialisation of the internal generator: lazy initialisation means there can be an arbitrary delay between creation of the context manager and the creation of the underlying generator that lets you know that you got the arguments wrong. By creating the generator immediately, any exceptions are thrown at the point where the context manager is created rather than when it is used. I think the cleanest way to fix this is to still create the generator automatically in __init__, but *also* save the original function and argument details. Then override __call__ to create a fresh instance of _GeneratorContextManager each time, instead of using "with self:" the way ContextDecorator does. The ContextDecorator documentation should also be updated to point out that it only works with reusable context managers. For 3.3, the fix could be generalised with ContextDecorator supporting a documented "self._recreate" internal interface that, by default, just returns self, but would be overridden in _GeneratorContextManager to actually create a new instance. The custom __call__ implementation for _GeneratorContextManager could then be removed. |
|||
msg132483 - (view) | Author: ysj.ray (ysj.ray) | Date: 2011-03-29 14:23 | |
> For 3.3, the fix could be generalised with ContextDecorator supporting a documented "self._recreate" internal interface that, by default, just returns self, but would be overridden in _GeneratorContextManager to actually create a new instance. The custom __call__ implementation for _GeneratorContextManager could then be removed. How about directly using the existing "copy.copy()" semantics instead of self._recreate()? That is, call "with copy.copy(self)" instead of "with self" in ContextGenerator.__call__(), implement ContextGenerator.__copy__() method to simply return self, and implement _GeneratorContextManager.__copy__() to create a copy of itself. This saves an internal interface. |
|||
msg132484 - (view) | Author: Alyssa Coghlan (ncoghlan) * | Date: 2011-03-29 15:24 | |
Because I don't want to conflate the two APIs. ContextDecorator can be used with *any* context manager, and some of those may already be using their copy semantics for something else. If anyone ever writes the __with__ PEP, then ContextDecorator._recreate can be updated to use that, but in the meantime an operation specific internal API is my preferred option. |
|||
msg132489 - (view) | Author: Michael Foord (michael.foord) * | Date: 2011-03-29 16:59 | |
I agree. copy is a separate protocol and shouldn't be involved here. |
|||
msg132585 - (view) | Author: ysj.ray (ysj.ray) | Date: 2011-03-30 13:19 | |
Got it. Here is my updated patch. Not sure if the doc is proper. |
|||
msg135204 - (view) | Author: Roundup Robot (python-dev) | Date: 2011-05-05 14:04 | |
New changeset e4ba097123f6 by Nick Coghlan in branch '3.2': Issue #11647: allow contextmanager objects to be used as decorators as described in the docs. Initial patch by Ysj Ray. http://hg.python.org/cpython/rev/e4ba097123f6 New changeset b23d1df7e47e by Nick Coghlan in branch 'default': Merge #11647 update from 3.2 http://hg.python.org/cpython/rev/b23d1df7e47e |
|||
msg135206 - (view) | Author: Alyssa Coghlan (ncoghlan) * | Date: 2011-05-05 14:09 | |
The core bug has been fixed for 3.2.1 and forward ported to 3.3. I credited the patch to "Ysj Ray" in ACKS and NEWS, let me know if you'd prefer a different attribution. Leaving the issue open for now, since _recreate_cm() should be renamed and documented for 3.3 (but I haven't settled on a public name at this point - recreate_cm() isn't quite right for the public API, as reusable CMs aren't actually recreated by the method) |
|||
msg149738 - (view) | Author: Alyssa Coghlan (ncoghlan) * | Date: 2011-12-18 07:25 | |
I'm prototyping a public version of the recreation API as "ContextDecorator.refresh_cm()" in contextlib2: http://contextlib2.readthedocs.org/en/latest/index.html#contextlib2.ContextDecorator.refresh_cm |
|||
msg161113 - (view) | Author: Alyssa Coghlan (ncoghlan) * | Date: 2012-05-19 12:52 | |
I'm closing this *without* converting ContextDecorator._recreate_cm() to a public method (although I'm also attaching the patch that would have done exactly that). My rationale for doing so is that I *still* consider making _GeneratorContextManager a subclass of ContextDecorator a design error on my part. Converting the existing _recreate_cm() hook to a public refesh_cm() method would be actively endorsing that mistake and encouraging others to repeat it. If anyone else wants to pursue this, create a new issue and be prepared to be very persuasive. After all, the current module maintainer just rejected his own implementation of the feature :) |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:57:15 | admin | set | github: 55856 |
2012-05-19 17:17:57 | eric.snow | set | nosy:
+ eric.snow |
2012-05-19 12:52:03 | ncoghlan | set | status: open -> closed files: + issue11647_refresh_cm.diff resolution: fixed messages: + msg161113 |
2011-12-18 07:25:45 | ncoghlan | set | type: behavior -> enhancement messages: + msg149738 |
2011-12-12 19:00:06 | giampaolo.rodola | set | nosy:
+ giampaolo.rodola |
2011-05-05 14:09:20 | ncoghlan | set | messages:
+ msg135206 versions: - Python 3.2 |
2011-05-05 14:04:03 | python-dev | set | nosy:
+ python-dev messages: + msg135204 |
2011-05-05 12:23:07 | ncoghlan | set | assignee: ncoghlan versions: + Python 3.2, Python 3.3 |
2011-03-30 13:19:52 | ysj.ray | set | files:
+ issue_11647_2.diff messages: + msg132585 |
2011-03-29 16:59:35 | michael.foord | set | messages: + msg132489 |
2011-03-29 15:24:31 | ncoghlan | set | messages: + msg132484 |
2011-03-29 14:23:57 | ysj.ray | set | messages: + msg132483 |
2011-03-29 10:30:48 | ncoghlan | set | messages: + msg132475 |
2011-03-29 08:33:33 | ysj.ray | set | files:
+ issue_11647.diff keywords: + patch messages: + msg132471 |
2011-03-28 14:49:18 | ysj.ray | set | messages: + msg132401 |
2011-03-26 00:15:21 | michael.foord | set | messages: + msg132189 |
2011-03-25 15:24:21 | pitrou | set | messages: + msg132100 |
2011-03-25 14:50:11 | ysj.ray | set | messages: + msg132096 |
2011-03-25 14:38:08 | pitrou | set | messages: + msg132094 |
2011-03-25 14:17:02 | ysj.ray | set | nosy:
+ ysj.ray messages: + msg132090 |
2011-03-24 05:07:32 | ncoghlan | set | messages: + msg131957 |
2011-03-23 23:39:12 | michael.foord | set | messages: + msg131944 |
2011-03-23 22:18:03 | pitrou | set | messages: + msg131931 |
2011-03-23 04:10:41 | ncoghlan | set | messages: + msg131851 |
2011-03-23 02:04:37 | pitrou | create |