Skip to content

Conversation

Kentzo
Copy link
Contributor

@Kentzo Kentzo commented Feb 12, 2018

Stub for changes added to Python 3.7 in python/cpython#4790.

@@ -64,3 +64,29 @@ if sys.version_info >= (3,):
def pop_all(self) -> ExitStack: ...
def close(self) -> None: ...
def __enter__(self: _U) -> _U: ...

if sys.version_info >= (3, 7):
from asyncio.futures import Future
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use typing.Awaitable instead of Future?

Copy link
Contributor Author

@Kentzo Kentzo Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at stubs for asyncio.tasks as an example.

On the second look I agree, Awaitable should be used instead.

*args: Any, **kwds: Any) -> Callable[..., None]: ...
def push_async_callback(self, callback: _CallbackCoroFunc,
*args: Any, **kwds: Any) -> _CallbackCoroFunc: ...
def pop_all(self) -> ExitStack: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return AsyncExitStack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think it should be implemented as __enter__ / __aenter__: pop_all instantiates type(self).

Probably that should also be fixed for ExitStack.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Would you mind fixing it there too while you're at it?

def pop_all(self) -> ExitStack: ...
def aclose(self) -> Awaitable[None]: ...
def __enter__(self: _U) -> _U: ...
def __aenter__(self: _U) -> Awaitable[_U]: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you not need __exit__ and __aexit__? I suppose the base class provides those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base class implementations should be enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that's true for __aexit__ but not __exit__.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? Definitions in typing.pyi seem legit (__exit__ / __aexit__)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you mean that's because AsyncContextManager does not inherit from ContextManager?

Copy link
Contributor Author

@Kentzo Kentzo Feb 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nvm. There is no __enter__ for AsyncExitStack.

@Kentzo
Copy link
Contributor Author

Kentzo commented Feb 17, 2018

@JelleZijlstra Should I squash into 2 commits: one for AsyncExitStack and one for ExitStack's bugfix?

@JelleZijlstra
Copy link
Member

No, that's fine. I'll squash when I merge (which I will do as soon as CI passes).

@JelleZijlstra JelleZijlstra merged commit 1e8b953 into python:master Feb 17, 2018
@JelleZijlstra JelleZijlstra mentioned this pull request Mar 18, 2018
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants