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

Complementary functions with concurrency control #70

Open
wants to merge 2 commits into
base: master
from

Conversation

@alexeyshockov
Copy link
Collaborator

alexeyshockov commented Jul 6, 2017

We have some handy functions to work with promises, but they don't support concurrency control.

This PR adds complementary functions with concurrent control (like each_limit()):

  • all_limit()
  • some_limit()
  • any_limit()
  • settle_limit()
@alexeyshockov

This comment has been minimized.

Copy link
Collaborator Author

alexeyshockov commented Jul 12, 2017

@mtdowling, @sagikazarmark, could you take a look?

@sagikazarmark

This comment has been minimized.

Copy link
Member

sagikazarmark commented Oct 6, 2017

@alexeyshockov sorry for the delay, I had a lot on my plate lately, but hopefully things will be a bit more relaxed. Will take a look at it soon.

@sagikazarmark sagikazarmark self-requested a review Oct 6, 2017
@RobberPhex

This comment has been minimized.

Copy link

RobberPhex commented Jan 30, 2018

Any new progress on this?

I think settle_limit is very helpful!

@alexeyshockov

This comment has been minimized.

Copy link
Collaborator Author

alexeyshockov commented Jan 30, 2018

The code itself works, waiting for the review.

@RobberPhex

This comment has been minimized.

Copy link

RobberPhex commented Feb 1, 2018

@sagikazarmark @stevenwadejr @Tobion

Is there anyone can review this?

@sagikazarmark sagikazarmark self-assigned this Feb 5, 2018
@sagikazarmark

This comment has been minimized.

Copy link
Member

sagikazarmark commented Feb 11, 2018

Sorry guys it took so long to review. Looks good to me.

@alexeyshockov can you please add some tests before merging?

@alexeyshockov alexeyshockov force-pushed the alexeyshockov:limit-functions branch from fd6b060 to 2d2f590 Aug 29, 2019
@GrahamCampbell GrahamCampbell mentioned this pull request Dec 31, 2019
* dynamic a concurrency size.
*
* @param mixed $promises Promises or values.
* @param int|callable $concurrency

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

$concurrency can also be null (at least you pass null in all_limit($promises, null, $recursive). so it should add the |null type.

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

zero 0 seems to mean the same thing. So we could instead also remove the nullable as well, esp. because null is not documented in the existing each_limit neither.

*
* @return PromiseInterface
*/
function all($promises, $recursive = false)
{
return all_limit($promises, null, $recursive);

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 12, 2020

Member

should pass 0 instead of null

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 12, 2020

I think instead of adding all these permutations of functions, it would be cleaner to have a builder for promises resolving where you can configure the mode like any, some and the concurrency independently.
This would be related to #108 by putting those functions in a builder instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.