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 annotation for Future._callbacks #2965

Merged
merged 2 commits into from
May 7, 2019

Conversation

mvismonte
Copy link
Contributor

Future._callbacks is suppose to be a List of Callable[[_S], Any]
according to:
https://github.com/python/cpython/blob/3.6/Lib/asyncio/futures.py#L159

add_done_callback on this class takes in the function that's appended
into _callbacks

Tested by running against IG's codebase and confirming that pyre now
acknowledges Future._callbacks

Note: I noticed that _callbacks and add_done_callback take slightly
different arguments for python 3.7. I can update the signature to be
compatible (it also takes in a context env) in a future PR.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks, two remarks.

mvismonte added 2 commits May 7, 2019 10:59
- Added support for python 3.7 annotation for `_callbacks`
- Changed `Sequence` to `List`
@mvismonte mvismonte force-pushed the typeshed-asyncio-future-callbacks branch from 60259d6 to 94a0d22 Compare May 7, 2019 15:27
@mvismonte
Copy link
Contributor Author

@srittau @JelleZijlstra I addressed the feedback. I also plan on making add_done_callback py3.7 compatible in a future PR.

@JelleZijlstra JelleZijlstra requested a review from srittau May 7, 2019 15:43
@srittau srittau merged commit 8451cd7 into python:master May 7, 2019
@mvismonte mvismonte deleted the typeshed-asyncio-future-callbacks branch May 7, 2019 16:24
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.

3 participants