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

Add a ContextManager ABC and type #69795

Closed
brettcannon opened this issue Nov 12, 2015 · 26 comments
Closed

Add a ContextManager ABC and type #69795

brettcannon opened this issue Nov 12, 2015 · 26 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir

Comments

@brettcannon
Copy link
Member

BPO 25609
Nosy @gvanrossum, @brettcannon, @rhettinger, @ncoghlan, @vadmium, @jstasiak
Dependencies
  • bpo-25637: Move non-collections-related ABCs out of collections.abc
  • bpo-26691: Update the typing module to match what's in github.com/python/typing
  • Files
  • contextmanagertype.diff
  • contextmanagertype.diff: Address Nick's comments
  • 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/brettcannon'
    closed_at = <Date 2016-04-15.17:52:06.015>
    created_at = <Date 2015-11-12.19:14:39.600>
    labels = ['easy', 'library']
    title = 'Add a ContextManager ABC and type'
    updated_at = <Date 2016-04-15.17:52:06.014>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2016-04-15.17:52:06.014>
    actor = 'brett.cannon'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2016-04-15.17:52:06.015>
    closer = 'brett.cannon'
    components = ['Library (Lib)']
    creation = <Date 2015-11-12.19:14:39.600>
    creator = 'brett.cannon'
    dependencies = ['25637', '26691']
    files = ['42209', '42279']
    hgrepos = []
    issue_num = 25609
    keywords = ['patch', 'easy']
    message_count = 26.0
    messages = ['254546', '254588', '254696', '254743', '254745', '257371', '257428', '257556', '262002', '262044', '262056', '262371', '262870', '262872', '262873', '262874', '262876', '262877', '262880', '263037', '263038', '263059', '263104', '263121', '263152', '263511']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'brett.cannon', 'rhettinger', 'ncoghlan', 'python-dev', 'martin.panter', 'jstasiak', 'Nan Wu', 'supriyanto maftuh']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue25609'
    versions = ['Python 3.5', 'Python 3.6']

    @brettcannon
    Copy link
    Member Author

    There should probably be a context manager ABC in collections.abc that requires enter/exit and a matching entry in typing that also takes a type argument of what the enter method returns.

    @brettcannon brettcannon added stdlib Python modules in the Lib dir easy labels Nov 12, 2015
    @NanWu
    Copy link
    Mannequin

    NanWu mannequin commented Nov 13, 2015

    Hi Brett, I'd like work on this feature. Your description here is clear. Besides that, could you give a use case of this context manager?

    @rhettinger
    Copy link
    Contributor

    -0 on this one. This is slightly more useful than the other "one-trick-ponies" like Container, Hashable, and Iterable. Those each provide some "recognition" capabilities using isinstance() but don't provide any useful "mixin" capabilities.

    In practice, I'm not seeing the "recognition" capabilities being used (i.e. no one is using "if isinstance(obj, Container)" in real code). So adding another one-trick pony that won't get used is a bit of a waste.

    Also, this would add more clutter to the collections ABCs, obscuring the ones that actually have added value (i.e. MutableMapping has been a great success). The collections.abc module has become a dumping ground for ABCs that that don't really have anything to do with collections and aren't useful for day-to-day development (Awaitable, Coroutine, AsyncIterable, AsyncIterator, etc).

    One idea is to have a separate module of generic recognition abcs that would be used in much the same way as the old types module. Otherwise, the collections.abc module will continue unabated growth away from its original purpose as a small group of powerful mixin classes (Sequence, MutableSequence, Set, MutableSet, Mapping, MutableMapping, and the dict views).

    @brettcannon
    Copy link
    Member Author

    The reason I suggested the ABC for context managers is mostly to provide a way to help people make sure to implement enter() and to provide a default exit() which has the proper signature (I personally always forget how many arguments exit() takes and what the arguments are past the first one). The type part of this feature request is because I realized that a type hint of "context manager" isn't really useful, but "context manager returning ..." is since a with statement does introduce a new variable. IOW none of this has anything to do with isinstance() and it's all about easing the use of the context manager interface and proper type hints for the with statement.

    As for a new module analogous to the types module just for ABCs, that's fine by me. I had the same reaction you did, Raymond, about putting it in collections.abc. I can open another issue for that idea and leave this open as it's somewhat orthogonal to what I'm proposing.

    @brettcannon
    Copy link
    Member Author

    Depending on how issue bpo-25637 turns out, the ABC could go into contextlib or a new interfaces modules.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 2, 2016

    What would your context manager base class do? I presume you supply a default __enter__() that does nothing, or perhaps just returns self.

    You mentioned having a default __exit__() but I am having trouble seeing how it would be useful. A context manager is only really useful if it does something interesting in __exit__(), and for that, the programmer has to write their own version and remember its signature.

    One option to simplify __exit__() that I used to use is a base class that defers to an abstract close() method with no parameters. But since ExitStack is now available, I am finding that is good enough instead.

    @gvanrossum
    Copy link
    Member

    The abstract base class would be an ABC providing documentation and a template for implementations, and a way to test using isinstance(). This is how several other ABCs are used that have no particularly useful default implementation.

    In addition, the presence of the ABC provides a reference for corresponding class in the typing module (used to express type hints using PEP-484) -- again this is just following the example of the other ABCs.

    @brettcannon
    Copy link
    Member Author

    In issue bpo-25637, Guido said he didn't want to move stuff out of collections.abc, so the context manager ABC will go into contextlib.

    To answer Martin's point, I could make __exit__ abstract to begin with to make people override it properly. The only reason I thought of providing a default implementation is that it inherently isn't required to be implemented to match the context manager interface/protocol. But as you pointed out, the usefulness of a context manager is derived from doing stuff in __exit__(), so I will make it abstract.

    @brettcannon brettcannon self-assigned this Jan 5, 2016
    @brettcannon
    Copy link
    Member Author

    Here is an initial patch to add AbstractContextManager and ContextManagerType to contextlib. There are no tests yet as I want to make sure this looks okay first.

    @ncoghlan
    Copy link
    Contributor

    • the ABC should have a structural __issubclass__ check looking for __enter__ and __exit__ methods

    • I agree with making __exit__ abstract (if you don't define it, you don't need a context manager), but does __enter__ need to be abstract? The "return self" default is a good one in many cases where the resource is acquired in __new__ or __init__.

    • the docs for the ABC are potentially confusing, as they describe what the default implementation does, without making it clear that it's only the default implementation and *permitted* return behaviour is more flexible than that.

    • I'd prefer not to have contextlib depend on typing, so the generic type definition should probably be in the typing module (similar to the abc-vs-type split for collections). That should also make backporting easier - the ABC can go back via contextlib2, while the generic type can go back via the comment-based typehinting variant

    @gvanrossum
    Copy link
    Member

    Agreed on all points with Nick. We can add ContextManager (not ContextManagerType) to typing.py in 3.5.2 (it's provisional).

    @brettcannon
    Copy link
    Member Author

    Here is an updated patch implementing Nick's suggestions and is ready for 3.6 sans a What's New entry.

    As for Python 3.5, I think I will copy __subclasshook__() from contextlib.AbstractContextManager and put it in typing.ContextManager so the isinstance() checks will work with the type structurally.

    LMK if this all sounds/looks reasonable.

    @gvanrossum
    Copy link
    Member

    A slight problem is that I'd really like to keep the source of typing.py identical in Python 3.5 and 3.6. So maybe the definition should be conditional on the presence of AbstractContextManager?

    @gvanrossum
    Copy link
    Member

    (Everything else looks great BTW.)

    @brettcannon
    Copy link
    Member Author

    When I have a chance I'll do up a new patch where the definition of typing.ContextManager is guarded, add a What's New entry, and commit it. My planned guard will be:

    if hasattr(contextlib, 'AbstractContextManager'):
        class ContextManager(...): ...
        __all__.append('ContextManager')

    @gvanrossum
    Copy link
    Member

    I have some significant changes to typing.py (and test_typing.py)
    upstream in https://github.com/python/typing/. We should coordinate
    our changes to stdlib (test_)typing.py.

    @brettcannon
    Copy link
    Member Author

    Is there a tracking issue I can set as a dependency?

    @gvanrossum
    Copy link
    Member

    No, but feel free to create one and assign it to me -- I will take
    care of the rest then.

    @brettcannon
    Copy link
    Member Author

    Tracker issue created and assigned.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 8, 2016

    New changeset 841a263c0c56 by Brett Cannon in branch 'default':
    Issue bpo-25609: Introduce contextlib.AbstractContextManager and
    https://hg.python.org/cpython/rev/841a263c0c56

    @brettcannon
    Copy link
    Member Author

    Thanks to everyone for the feedback! I will now go backport this to python/typing on GitHub.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 9, 2016

    The docs buildbot is complaining:

    http://buildbot.python.org/all/builders/Docs%203.x/builds/1156/steps/lint/logs/stdio
    [2] whatsnew/3.6.rst:199: default role used

    I guess this is complaining about the `__enter__()` syntax with back-ticks. Maybe it should be either :meth:`__enter__` or ``__enter__()``, etc.

    @brettcannon
    Copy link
    Member Author

    The typing changes need to be backported to 3.5 as-is to keep the files in sync.

    @brettcannon brettcannon reopened this Apr 9, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 10, 2016

    New changeset 5c0e332988b2 by Martin Panter in branch 'default':
    Issue bpo-25609: Double back-ticks to avoid “make check” buildbot failure
    https://hg.python.org/cpython/rev/5c0e332988b2

    @brettcannon
    Copy link
    Member Author

    Thanks for fixing the doc bug, Martin. Too much Markdown lately. :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 15, 2016

    New changeset 257dd57dd732 by Brett Cannon in branch '3.5':
    Issue bpo-25609: Backport typing.ContextManager.
    https://hg.python.org/cpython/rev/257dd57dd732

    New changeset 14cf0011c5e9 by Brett Cannon in branch 'default':
    Merge for issue bpo-25609
    https://hg.python.org/cpython/rev/14cf0011c5e9

    @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
    easy stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants