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.

classification
Title: contextlib.ExitStack.__enter__ has trivial but undocumented behavior
Type: Stage: resolved
Components: Documentation Versions: Python 3.11
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: JelleZijlstra, andrei.avk, docs@python, martin.panter, miss-islington, r.david.murray, vidhya, walker.hale.iv
Priority: normal Keywords: easy, patch

Created on 2016-10-24 02:36 by walker.hale.iv, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
issue28516.diff walker.hale.iv, 2016-10-24 18:25 review
Pull Requests
URL Status Linked Edit
PR 31636 merged vidhya, 2022-03-01 15:49
PR 32144 closed miss-islington, 2022-03-28 04:31
PR 32145 merged miss-islington, 2022-03-28 04:31
PR 32171 merged JelleZijlstra, 2022-03-29 02:11
Messages (14)
msg279296 - (view) Author: Walker Hale IV (walker.hale.iv) * Date: 2016-10-24 02:36
contextlib.ExitStack implies but does not explicitly state that its __enter__ method trivially returns self.

This means that if a user invokes pop_all and then uses the resulting ExitStack instance in a with statement, the user will be relying on undocumented behavior. Avoiding undocumented behavior forces the user to instead use a tedious try/finally construct, partially defeating the elegance of context managers.

I propose that:

1. The ExitStack.__enter__ method be briefly mentioned as doing nothing besides returning self.

2. The example in pop_all documentation be expanded to show a following with statement that uses the new ExitStack instance.

The discussion in section 29.6.3.2 is not sufficient to make this trivial point clear.
msg279311 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-10-24 15:02
Hmm.  I guess we just assume it is obvious, given that the with statement examples name the variable 'stack', and the pop_all docs mention that no callbacks are called until it is closed "or at the end of a with statement".  So, technically, using the result of a pop_all in a with statement is documented.  I could be made clearer though if someone wants to propose a doc patch.  I'm not sure it is worth expanding the example, though.
msg279326 - (view) Author: Walker Hale IV (walker.hale.iv) * Date: 2016-10-24 18:36
This one-line patch should clarify the point.
msg279463 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-25 22:48
I agree it is good to explicitly document the __enter__() result, rather than relying on assumptions and example code. The patch looks good to me.

I don’t understand the problem with pop_all() though. Is there still a problem if we apply your patch?
msg279467 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-10-26 00:15
Actually, the __enter__ method is looked up on the class, so saying "the __enter__ method of the instance" could be a bit confusing.  Also, many context managers return self, so 'trivially' isn't really necessary as a modifier.

What if we added a sentence to the first paragraph that said "ExitStack instances return themselves when used in a with statement."  Since that is immediately followed by an example of doing that, it seems like the best place to put it.
msg279486 - (view) Author: Walker Hale IV (walker.hale.iv) * Date: 2016-10-26 03:13
Clarifying the documentation regarding the __enter__ method eliminates the need for further discussion on this point regarding pop_all(), which was really just the motivating use case.

That leaves the question of the most readable documentation change that accomplishes the result — some point between verbose and terse that minimizes the time required to comprehend the material.

My problem with the language "ExitStack instances return themselves when used in a with statement" is that it only specifies the return value of the __enter__ method but leaves open the question of whether the __enter__ method is doing anything else, particularly in the case of an ExitStack that is already loaded with context managers. How does a reader know that the __enter__ method of a loaded ExitStack doesn't call the __enter__ method of the the context managers inside? The documentation elsewhere provides strong evidence against this, but nothing that makes the point explicit. The reader is left with an exercise in deduction.

How about replacing my previous wording with: "The __enter__ method has no behavior besides returning the ExitStack instance?"

(I feel a little dirty using that language, since it might tie the hands of future developers. The truly correct wording would be "The __enter__ method is idempotent and behaves as if the only thing it does is return the ExitStack instance." That more verbose description gives future developers the freedom to do weird JIT optimizations and caching as needed, but who has the patience for such legally exhaustive specification?)

Placing the wording where I did — at the end of the class discussion and prior to the new methods — prevents this point from obscuring the main purpose of ExitStack while still leaving a place for such messy but important details. (Amazing the thought that goes into documenting a two-line method.)
msg407867 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-12-06 21:57
It's a bit more readable to start by stating what a function does rather than what it doesn't do. I also wouldn't worry about possible future minor optimizations that might be added, because if that happens, a simple ".. aside from internal optimizations, ... " would suffice.

Something like this should work:

The __enter__ method returns the ExitStack instance, and performs no additional operations.

 OR

 ... and does not perform any additional operations.
msg414232 - (view) Author: Vidhya (vidhya) * Date: 2022-03-01 01:54
[Entry level contributor seeking guidance] If this is still open, I can update the document.

After reading all the above comments, planning to add 

"The __enter__ method returns the ExitStack instance, and performs no additional operations." 

at

https://docs.python.org/3/library/contextlib.html#contextlib.ExitStack.enter_context. 

Please let me know your thoughts.
msg414235 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-03-01 03:00
That sounds good. Feel free to request review from me if you make a PR.
msg414276 - (view) Author: Vidhya (vidhya) * Date: 2022-03-01 15:49
@Jelle: Thanks. The PR is submitted at
https://github.com/python/cpython/pull/31636
msg416144 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-03-28 04:31
New changeset 86384cf83f96fcaec03e2ad6516e2e24f20d3b92 by vidhya in branch 'main':
bpo-28516: document contextlib.ExitStack.__enter__ behavior (GH-31636)
https://github.com/python/cpython/commit/86384cf83f96fcaec03e2ad6516e2e24f20d3b92
msg416202 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-03-28 18:45
New changeset 1e3132b1c3ebff8d28a6dd353bf217cb97c41e81 by Miss Islington (bot) in branch '3.9':
bpo-28516: document contextlib.ExitStack.__enter__ behavior (GH-31636) (GH-32145)
https://github.com/python/cpython/commit/1e3132b1c3ebff8d28a6dd353bf217cb97c41e81
msg416239 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-03-29 02:40
New changeset 604d003ab4d1084ef828ebca1b28f2bf1b93c744 by Jelle Zijlstra in branch '3.10':
[3.10] bpo-28516: document contextlib.ExitStack.__enter__ behavior (GH-31636) (GH-32171)
https://github.com/python/cpython/commit/604d003ab4d1084ef828ebca1b28f2bf1b93c744
msg416240 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-03-29 02:58
Thanks for the patch!
History
Date User Action Args
2022-04-11 14:58:38adminsetgithub: 72702
2022-03-29 02:58:07JelleZijlstrasetstatus: open -> closed
resolution: fixed
messages: + msg416240

stage: patch review -> resolved
2022-03-29 02:40:03JelleZijlstrasetmessages: + msg416239
2022-03-29 02:11:40JelleZijlstrasetpull_requests: + pull_request30249
2022-03-28 18:45:51JelleZijlstrasetmessages: + msg416202
2022-03-28 04:31:59JelleZijlstrasetmessages: + msg416144
2022-03-28 04:31:50miss-islingtonsetpull_requests: + pull_request30225
2022-03-28 04:31:44miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request30224
2022-03-01 15:49:12vidhyasetkeywords: + patch

messages: + msg414276
pull_requests: + pull_request29758
2022-03-01 03:00:35JelleZijlstrasetnosy: + JelleZijlstra
messages: + msg414235
2022-03-01 01:54:54vidhyasetnosy: + vidhya
messages: + msg414232
2021-12-06 21:57:49andrei.avksetnosy: + andrei.avk
messages: + msg407867
2021-06-18 22:34:41iritkatrielsetkeywords: + easy, - patch
versions: + Python 3.11, - Python 3.5, Python 3.6, Python 3.7
2016-10-26 03:13:38walker.hale.ivsetmessages: + msg279486
2016-10-26 00:15:31r.david.murraysetmessages: + msg279467
2016-10-25 22:48:56martin.pantersetversions: - Python 3.3, Python 3.4
nosy: + martin.panter

messages: + msg279463

stage: patch review
2016-10-24 18:36:57walker.hale.ivsetmessages: + msg279326
2016-10-24 18:25:24walker.hale.ivsetfiles: + issue28516.diff

nosy: + docs@python
assignee: docs@python
components: + Documentation
keywords: + patch
2016-10-24 15:02:56r.david.murraysetnosy: + r.david.murray
messages: + msg279311
2016-10-24 02:36:51walker.hale.ivcreate