bpo-10049: Add a "no-op" (null) context manager to contextlib by Jesse-Bakker · Pull Request #4464 · python/cpython

Jesse-Bakker · 2017-11-19T17:46:10Z

bpo-10049: Add a "no-op" (null) context manager to contextlib
Removed section in docs using ExitStack() for no-op context manager

https://bugs.python.org/issue10049

merwok · 2017-11-19T18:32:43Z

Lib/contextlib.py

+class nullcontext(AbstractContextManager):
+    """Context manager that does no additional processing.
+
+    Used as a standin for a normal context manager, when a particular block of code is only sometimes used with a normal context manager:


Please wrap long lines.

Typo: stand-in

merwok · 2017-11-19T18:33:52Z

Doc/library/contextlib.rst

+            return nullcontext(details)
+
+        with debug_trace(details) as d:
+            # Suite is traced in debug mode, but runs normally otherwise


Can you check if this code block is rendered correctly? Since comments are stripped out and do not become statements, this may be invalid code. If that is so (I’m not sure!), adding an ellipsis would fix it.

It is rendered correctly. I am unsure about the example though. I used the example previously used for the recipe using ExitStack and could not think of a better one. I don't find it all too intuitive though.

Maybe there was a better use case in the ticket?

An example I encountered was having to close a file if I opened it in the function, but not if a file object was passed to the function. This could read:

def function(target): """"Do something with target (file object or path as string.""" if isinstance(target, str): # or FileLike cm = open(target) else: # Wrap the file in a no-op context manager, the caller will close it cm = nullcontext(target) with cm as file: print('hello', file=file)

Thanks for the suggestion. Changed the docs.

merwok · 2017-11-21T18:49:43Z

Doc/library/contextlib.rst



-Simplifying support for single optional context managers
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


I wonder if the section heading should be kept but the text changed to direct to nullcontext, so that people following links can see the new helper.

Also, if this is the only place that explained that ExitStack can work as a no-op, I think a line about that should be kept.

Yes, it definitely should be kept.

I agree we still need the hyperlink target, but given the clear example in the function's documentation, I think we can still delete the recipe section.

merwok · 2017-11-21T18:50:27Z

Doc/library/contextlib.rst

   ``page.close()`` will be called when the :keyword:`with` block is exited.


+.. function:: nullcontext(thing=None)


Minor: I’m nonplussed by the param name thing, I can’t think of a precedent in the stdlib.

obj is probably more common.

For this, I'd ask that we make the name explicit and call it enter_result.

warsaw · 2017-11-21T19:06:28Z

Doc/library/contextlib.rst

   ``page.close()`` will be called when the :keyword:`with` block is exited.


+.. function:: nullcontext(thing=None)


obj is probably more common.

warsaw · 2017-11-21T19:07:21Z

Doc/library/contextlib.rst



-Simplifying support for single optional context managers
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


Yes, it definitely should be kept.

warsaw · 2017-11-21T19:07:56Z

Misc/NEWS.d/next/Library/2017-11-19-18-42-31.bpo-10049.U91c0A.rst

@@ -0,0 +1 @@
+Added nullcontext no-op context manager to contextlib


Can you please make this a complete sentence?

I think the main missing piece here is the explanation of the user benefit. Something like:

Added nullcontext no-op context manager to contextlib. This provides a simpler and faster alternative to ExitStack() when handling optional context managers.

ncoghlan

Thanks! The general structure here looks good to me, but I'd prefer we used the more descriptive parameter and attribute name enter_result.

There's also a small docs tweak to ensure links to the old recipe section are automatically directed to the new context manager

ncoghlan · 2017-11-22T07:57:46Z

Doc/library/contextlib.rst



-Simplifying support for single optional context managers
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


I agree we still need the hyperlink target, but given the clear example in the function's documentation, I think we can still delete the recipe section.

ncoghlan · 2017-11-22T08:01:03Z

Doc/library/contextlib.rst

   ``page.close()`` will be called when the :keyword:`with` block is exited.


+.. function:: nullcontext(thing=None)


For this, I'd ask that we make the name explicit and call it enter_result.

ncoghlan · 2017-11-22T08:02:50Z

Doc/library/contextlib.rst


+.. function:: nullcontext(thing=None)
+
+    Return a context manager that just returns *thing*. It is intended to be used


Adjust the first sentence to be:

Return a context manager that returns enter_result from __enter__, but otherwise does nothing.

ncoghlan · 2017-11-22T08:04:07Z

Doc/library/contextlib.rst

@@ -137,6 +137,23 @@ Functions and classes provided:
   ``page.close()`` will be called when the :keyword:`with` block is exited.



Declare an explicit hyperlink anchor here with a blank line before the function definition:

.. _simplifying-support-for-single-optional-context-managers: .. function:: nullcontext(enter_result=None)

ncoghlan · 2017-11-22T08:06:23Z

Lib/contextlib.py

+        # Perform operation, using optional_cm if condition is True
+    """
+
+    def __init__(self, thing=None):


As per the docs comment, replace thing with enter_result for both the parameter and attribute name.

ncoghlan · 2017-11-22T08:09:09Z

Misc/NEWS.d/next/Library/2017-11-19-18-42-31.bpo-10049.U91c0A.rst

@@ -0,0 +1 @@
+Added nullcontext no-op context manager to contextlib


I think the main missing piece here is the explanation of the user benefit. Something like:

Added nullcontext no-op context manager to contextlib. This provides a simpler and faster alternative to ExitStack() when handling optional context managers.

ncoghlan · 2017-11-22T08:11:45Z

Doc/library/contextlib.rst

+            with cm as file:
+                # Perform processing on the file
+
+


This needs a version added marker:

.. versionadded: 3.7

bedevere-bot · 2017-11-22T08:12:05Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Jesse-Bakker · 2017-11-22T16:32:17Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-22T16:32:19Z

Thanks for making the requested changes!

@merwok, @ncoghlan: please review the changes made to this pull request.

merwok · 2017-11-22T17:06:12Z

Doc/library/contextlib.rst

-    as a stand-in for an optional context manager, for example::
+.. function:: nullcontext(enter_result=None)
+
+    Return a context manager that returns enter_result from ``__enter__``, but


rst blocks are indented with three spaces

ncoghlan

Thanks! Once the docs directive is fixed, I think this will be ready to merge.

ncoghlan · 2017-11-22T23:50:10Z

Doc/library/contextlib.rst

+          with cm as file:
+              # Perform processing on the file
+
+   .. versionadded: 3.7


Sorry, my suggested syntax here wasn't quite right (and that's what the CI build is complaining about). The directive needs a second colon:

.. versionadded:: 3.7

ncoghlan · 2017-11-23T00:24:06Z

Thanks for the patch!

Jesse Bakker added 2 commits November 19, 2017 18:40

Add nullcontext to contextlib

3cc01ab

Add nullcontext to contextlib

ca4d97e

the-knights-who-say-ni added the CLA signed label Nov 19, 2017

bedevere-bot added the awaiting review label Nov 19, 2017

merwok reviewed Nov 19, 2017

View reviewed changes

merwok self-requested a review November 19, 2017 18:34

Jesse Bakker added 3 commits November 19, 2017 19:48

Docs typo and formatting

a194b9c

Fix whitespace

23cc840

Change example in docs to a more common/intuitive one

9373422

Jesse-Bakker closed this Nov 21, 2017

Jesse-Bakker reopened this Nov 21, 2017

merwok assigned ncoghlan Nov 21, 2017

merwok approved these changes Nov 21, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Nov 21, 2017

warsaw reviewed Nov 21, 2017

View reviewed changes

ncoghlan requested changes Nov 22, 2017

View reviewed changes

bedevere-bot removed the awaiting merge label Nov 22, 2017

bedevere-bot added the awaiting changes label Nov 22, 2017

Edited lib and docs according to code review by @ncoghlan

7e6f26e

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 22, 2017

merwok reviewed Nov 22, 2017

View reviewed changes

Jesse Bakker added 2 commits November 22, 2017 18:15

Fixed docs indentation

dec5304

Fix docs indentation

89ffdef

merwok approved these changes Nov 22, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 22, 2017

ncoghlan approved these changes Nov 22, 2017

View reviewed changes

Fix versionadded directive

9947ded

ncoghlan merged commit 0784a2e into python:master Nov 23, 2017

bedevere-bot removed the awaiting merge label Nov 23, 2017



		Simplifying support for single optional context managers
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

		``page.close()`` will be called when the :keyword:`with` block is exited.


		.. function:: nullcontext(thing=None)

		@@ -0,0 +1 @@
		Added nullcontext no-op context manager to contextlib


		.. function:: nullcontext(thing=None)

		Return a context manager that just returns thing. It is intended to be used

		@@ -137,6 +137,23 @@ Functions and classes provided:
		``page.close()`` will be called when the :keyword:`with` block is exited.

Uh oh!

Conversation

Jesse-Bakker commented Nov 19, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 22, 2017

Uh oh!

Jesse-Bakker commented Nov 22, 2017

Uh oh!

bedevere-bot commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan commented Nov 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Jesse-Bakker commented Nov 19, 2017 •

edited by bedevere-bot

Loading