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

Initial implementation of Promises.all? and Promise.any? class methods. #198

Merged
merged 5 commits into from
Dec 11, 2014

Conversation

jdantonio
Copy link
Member

A Promise.all method was originally proposed by @gdo in Issue #80. This request was later echoed by @dinshaw when he and I met at RubyConf 2014. @dinshaw provided spike code with tests indicating one possible implementation.

This PR implements four aggregating methods: all, any, none, and one that provide behavior based on the similarly-named methods in Ruby's Ennumerable module. It generalizes that behavior in an aggregate method that could (potentially) be extended and enhanced.

composite = Promise.new do
completed = promises.collect do |promise|
promise.execute if promise.unscheduled?
promise.wait
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about doing zip this way, but it seems to me this locks up a thread for no good reason (at least in some cases). Should be possible to do the same just with registering hooks and on_reject/on_fulfil

Copy link
Member Author

Choose a reason for hiding this comment

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

This probably isn't the most efficient implementation. There is definitely room for refactoring once we nail down the API.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 26ae888 on promise-all into df20680 on master.

@jdantonio jdantonio changed the title Initial implementation of Promises.all, .any, .none, and .one class methods Initial implementation of Promises.all?, .any?, .none?, and .one? class methods Dec 7, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) when pulling 2fe40b9 on promise-all into df20680 on master.

@pitr-ch
Copy link
Member

pitr-ch commented Dec 8, 2014

FYI new future/promise #176 contains efficient implementation of all and any. one and none are interesting ideas, I'll generalize it to any number of successes where one and none will be shortcuts.

@jdantonio
Copy link
Member Author

@pitr-ch Thank you! I saw your post that the #176 branch includes any/all. Since this functionality was requested by someone I met at RubyConf who wants to use our gem in production, I wanted to get his feedback on the proposed API. I'd like to hold off a couple of days and see if we can get some feedback before we make the final decision on this API.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 36548df on promise-all into d5b0736 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) when pulling bf0bc6f on promise-all into 67d6631 on master.

@pitr-ch
Copy link
Member

pitr-ch commented Dec 9, 2014

I liked one and none, but now I am thinking about it again. Should we promote failures of the promises? They are basically equal to exceptions so this feels little bit like control flow by exceptions. We could have all, any, and first(futures, count = 1) where first would collect count of first successful futures. All would fail on first failure. That feels better to me. What do you think?

@jdantonio
Copy link
Member Author

I don't have strong opinions about one and none. The request was for any and all. I only added one and none for synergy with Enumerable (and because I thought it was clever--never a good sign). I like your first suggestion, but we don't have a use case yet (we don't have a use case for one and none, either). Since these methods are all additive it may be better for use to just add any and all for now and keep our options open for the future. It's much easier to add methods later then remove or change them. In retrospect, I may have been getting carried away by adding one and none.

The original request was for .any? and .all? methods. We added .none?
and .one? methods for synergy with arrays, but didn't like that these
methods supressed exceptions. So we decided to add only the requested
methods and consider other methods in the future.
@jdantonio jdantonio changed the title Initial implementation of Promises.all?, .any?, .none?, and .one? class methods Initial implementation of Promises.all? and Promise.any? class methods. Dec 11, 2014
jdantonio added a commit that referenced this pull request Dec 11, 2014
Initial implementation of Promises.all? and Promise.any? class methods.
@jdantonio jdantonio merged commit 29cd69a into master Dec 11, 2014
@jdantonio jdantonio deleted the promise-all branch December 11, 2014 03:44
@jdantonio jdantonio mentioned this pull request Dec 11, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+3.99%) when pulling 4fb4b7d on promise-all into 67d6631 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+3.99%) when pulling 4fb4b7d on promise-all into 67d6631 on master.

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.

4 participants