-
Notifications
You must be signed in to change notification settings - Fork 418
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
Promise.zip execution changes #654
Promise.zip execution changes #654
Conversation
Thanks. The change makes sense but as you said it's a breaking change. Could you introduce the non-executing behavior with an option passed to zip? |
@pitr-ch I added an |
@davishmcclurg Thanks for the update, looks good. One last think though, could you update the documentation to mention the new option? |
@pitr-ch I finally got around to updating the docs. I believe |
It's unnecessary to immediately execute promises that are chained together with `zip`. This feels like a bug to me, but I'm sure people are relying on the old behavior, so this is definitely a breaking change.
The use case for this is a little strange, since you can set the executors for the zipped promises when they're created instead. In my case, it would make it easier to run a number of immediately executed promises on a pool: ``` p1 = Concurrent::Promise.new(executor: :immediate) { ... } p2 = Concurrent::Promise.new(executor: :immediate) { ... } ... Concurrent::Promise.zip(p1, p2, executor: SOME_THREAD_POOL).then { ... } ```
This restores the default behavior for zipped promises and adds an option (`execute`) to leave them unscheduled.
5745bef
to
0559cba
Compare
Thanks looks great! I've rebased the PR to pass the CI. When green I'll merge it. |
I added details for each change in the commit messages.
Let me know if you'd like these in separate PRs.