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 Promise.all utility method. #9

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

dylanahsmith
Copy link
Collaborator

@eapache for review

This PR adds an implementation of the Promise.all javascript method which can be useful to wait for the result of multiple promises.

@promise = Promise.new
@inputs = inputs
@on_fulfill = method(:on_fulfill)
@on_reject = promise.method(:reject)
Copy link

Choose a reason for hiding this comment

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

these don't need to be instance vars, they're only used the once in chain_inputs and are never mutated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, I just did this for memoization since each call to method returns a new object.

Copy link

Choose a reason for hiding this comment

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

they can just be locals in chain_inputs though, they don't need to hang around

@eapache
Copy link

eapache commented Feb 9, 2016

Other than the issue in chain_inputs this looks fine. Congratulations on your unexpected maintainership of the project :)

@dylanahsmith
Copy link
Collaborator Author

I've addressed your feedback if you want to give it another look.

obj.instance_of?(Promise)
end

def count_promises(enumerable)
Copy link

Choose a reason for hiding this comment

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

this and each_promise don't need to take a param, they can just operate on @inputs; otherwise they belong as class methods

@eapache
Copy link

eapache commented Feb 9, 2016

One nit, and CI seems to think you've lost a line of code coverage somewhere, but otherwise LGTM

@dylanahsmith
Copy link
Collaborator Author

The mutation test coverage seems to be flaky. The false positives seem to show up as a "Neutral failure", but I don't know how to ignore them. They also seem to be timeout errors, so I think it is from the timeout devtools uses as recommended in the mutant README.

I might just disable that check on CI until I figure out how to make it more reliable.

@dylanahsmith dylanahsmith force-pushed the promise.all branch 2 times, most recently from eb55fc4 to 58f429a Compare February 9, 2016 17:56
Based on the Promise.all javascript method.

Closes lgierth#9
@dylanahsmith dylanahsmith merged commit 0f0d696 into lgierth:master Feb 9, 2016
@dylanahsmith dylanahsmith deleted the promise.all branch February 9, 2016 18:03
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