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

Added when(maxFulfilledCount:) #1123

Closed
wants to merge 4 commits into from
Closed

Added when(maxFulfilledCount:) #1123

wants to merge 4 commits into from

Conversation

RomanPodymov
Copy link
Collaborator

Hello.
Thank you for PromiseKit.
In this pull request I added a new parameter to some when versions. With this parameter when behaves like race with the amount of winners.

@mxcl
Copy link
Owner

mxcl commented Jan 19, 2020

  1. What is this for? Please provide concrete examples of where it is useful
  2. We should not overload when with this, unless it makes sense to, but I don’t see how it makes sense idiomatically as yet

As a note, generally it is advisable to discuss adding features to a library that is almost 10 years old before investing time making a PR. This PR is high quality, but the reason PMK is high quality is because we are careful about additions.

@RomanPodymov
Copy link
Collaborator Author

@mxcl I was thinking about the solution for #1114. As for me there should be a function like when(resolved:maxFulfilledCount:) and it should be called like when(resolved: [fulfilledPromise, errorPromise, pendingPromise], maxFulfilledCount: 1). In this pull request I added a similar function that can be also useful. Could you please explain more properly how can we resolve #1114 without a new function?

@mxcl
Copy link
Owner

mxcl commented Jan 27, 2020

I feel a variant of race is more what you want to provide, perhaps race(fulfilled:) which implies it allows failures and waits for some promise to actually win.

@RomanPodymov
Copy link
Collaborator Author

RomanPodymov commented Jan 28, 2020

Hello @mxcl
I opened a pull request for race(fulfilled:).

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