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

Promise.fold() #47

Merged
merged 6 commits into from
Dec 2, 2020
Merged

Promise.fold() #47

merged 6 commits into from
Dec 2, 2020

Conversation

oltrep
Copy link
Contributor

@oltrep oltrep commented Nov 20, 2020

Closes #46

As discussed in the issue, here is the PR for Promise.fold!

Let me know if you have any comments/improvements :)

@jhampton
Copy link

Thank you @oltrep! I'm curious - is there a cancellation behavior that needs to be supported here from the reference implementation (or to align with the other utilities in this library).

@evaera
Copy link
Owner

evaera commented Nov 24, 2020

Thank you for the PR! I will review this next week. I'm on vacation this week

Copy link
Owner

@evaera evaera left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have requested some changes. Notably, we should always return a Promise from Promise.fold, so as to be in accordance with the other functions and for consistency.

You can take advantage of chaining so you don't have to inspect the values directly: https://eryn.io/roblox-lua-promise/lib/Tour.html#chaining

This function also has no cancellation semantics. You can read about cancellation here: https://eryn.io/roblox-lua-promise/lib/Tour.html#cancellation

If we instead implement Promise.fold over top of Promise.each, we get those cancellation and chaining semantics for free. How does that sound?

Thanks again!

@oltrep
Copy link
Contributor Author

oltrep commented Dec 1, 2020

I think I've addressed all your comments, let me know if you find something else :)

Copy link
Owner

@evaera evaera left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions! Looks good.

@evaera evaera merged commit 48d756b into evaera:master Dec 2, 2020
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.

Promise.reduce(array, mapFn, initialValue)
3 participants