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

feat: add coroutine::await_in_coroutine to await awaitables in coroutine context #3611

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Nov 30, 2023

Relates to #1632

Draft based on #3610

Copy link

codspeed-hq bot commented Nov 30, 2023

CodSpeed Performance Report

Merging #3611 will not alter performance

Comparing wyfo:pyfuture (1bf4f4c) with main (7263fa9)

🎉 Hooray! pytest-codspeed just leveled up to 2.2.1!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

✅ 67 untouched benchmarks

@wyfo wyfo force-pushed the pyfuture branch 17 times, most recently from d1e9e17 to 53cccb4 Compare December 7, 2023 19:17
@davidhewitt
Copy link
Member

@wyfo I'm sorry for the painfully slow review here.

Given that allow_threads is currently under intense scrutiny and we haven't yet decided on a concrete solution, #3610 risks getting stuck for a bit while we figure out a way forward for that API.

Does this patch rely on #3610 getting merged, or is it possible to rebase this on main? That would allow us to move forward with some of these remaining async PRs.

@wyfo
Copy link
Contributor Author

wyfo commented Apr 7, 2024

There is one thing bothering me with this implementation: the name PyFuture. In fact, there is Future object in Python, and it doesn't match the underlying type of PyFuture.
PyFuture is the result of calling __await__, so it's an iterator, but that's all we know, and I don't know any naming convention for this object – PEP 492 doesn't give it a name.

I've chosen PyFuture because it sounds like Rust Future, but that's maybe not a good reason. If you have some name suggestion, I'm interested.

@wyfo wyfo force-pushed the pyfuture branch 2 times, most recently from e15675f to 053bfc2 Compare April 7, 2024 09:40
@wyfo
Copy link
Contributor Author

wyfo commented Apr 7, 2024

The error in CI (other than non_local_definition lint) is due to the bug mentioned in #4055.

@wyfo wyfo force-pushed the pyfuture branch 3 times, most recently from 2ebb268 to 18f59ea Compare April 25, 2024 09:20
@wyfo wyfo changed the title feat: add PyFuture to await Python awaitables feat: add coroutine::await_in_coroutine to await awaitables in coroutine context May 7, 2024
@wyfo
Copy link
Contributor Author

wyfo commented May 7, 2024

As written above, the name PyFuture was not a good name (the Python object doesn't even have a name, see this discussion), so I dropped it here to reuse it in #4057 where it's more suited.
Instead, I chose the more explicit name await_in_coroutine, making it easier to document, and for the user to understand that it must be used in coroutine context.

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