-
-
Notifications
You must be signed in to change notification settings - Fork 21
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 wait_for
and wait_for_value
to WaitCell and WaitQueue
#479
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you! I left some feedback, but it's mostly just me being pedantic about docs, plus some style suggestions that you're welcome to take if you like them, or to ignore if you don't.
It might be nice to have tests and/or examples for the new code, but it's simple enough that I'm happy to merge this regardless. We may want to add tests asserting that the futures returned by these functions are Send
/Sync
when the input closure is Send
/Sync
, though, so that future internal changes don't introduce an accidental API compatibility breakage.
Co-authored-by: Eliza Weisman <eliza@elizas.website>
Co-authored-by: Eliza Weisman <eliza@elizas.website>
Co-authored-by: Eliza Weisman <eliza@elizas.website>
github was being weird about applying suggestions, not sure why it thought they were overlapping. I plan to come back to add docs and address the remaining open topics |
Great, I'll be happy to merge this whenever the docs are ready (and ideally, when we can get a clean CI build on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs and tests look good to me --- happy to merge this once CI is green!
@hawkw tests are green (🎉), let me know what you think re: |
merged, thank you! |
Implementing the interface that @hawkw so wisely suggested