Promise.zip execution changes #654
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pitr-ch
added
the
enhancement
Member
|
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? |
Contributor
Author
|
@pitr-ch I added an |
Member
|
@davishmcclurg Thanks for the update, looks good. One last think though, could you update the documentation to mention the new option? |
Contributor
Author
|
@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.
pitr-ch
force-pushed
the
davishmcclurg/promise-zip
branch
from
5745bef to
0559cba
Compare
ghost
assigned
Member
|
Thanks looks great! I've rebased the PR to pass the CI. When green I'll merge it. |
pitr-ch
approved these changes
Feb 24, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added details for each change in the commit messages.
Let me know if you'd like these in separate PRs.