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

add mergeLimit function as mentioned in #374 #375

Merged
merged 6 commits into from
Sep 20, 2015
Merged

Conversation

dayoadeyemi
Copy link
Contributor

No description provided.

@vqvu
Copy link
Collaborator

vqvu commented Sep 13, 2015

This breaks backpressure. You should be using modified version of the current implementation of merge. Then have merge return this.mergeLimit(Infinity).

You all so need to add a noValueOnErrorTest. See the other tests for details.

*
* _([txt, md, js]).mergeLimit(2);
* // => contents of foo.txt, bar.txt, baz.txt and bosh.js in the order
* they were read, but bosh is not read until the others have comleted
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in "comleted". Also, this wording suggests bosh won't be read until both bar and baz have completed. I suggest "but bosh is not read until either foo/bar or baz has finished.

@vqvu
Copy link
Collaborator

vqvu commented Sep 13, 2015

Finally, let's call this mergeWithLimit to match the existing batchWIthTimeOrCount.

@dayoadeyemi dayoadeyemi force-pushed the master branch 2 times, most recently from 649928f to 44464fc Compare September 15, 2015 00:49
@dayoadeyemi
Copy link
Contributor Author

Yes that did break back-pressure, Instead of modifying the merge function I've made it so that there is just a transform function before the merge which prevents the stream from pushing any values until something from the current working set has ended.

push(err, x);
// console.log('start', x.id)
x.once('end', function(){
// console.log('end', x.id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your indentation for x.once is off. Also, I think you want to do index-- in the end handler, right?

@vqvu
Copy link
Collaborator

vqvu commented Sep 15, 2015

I've made it so that there is just a transform function before the merge which prevents the stream from pushing any values until something from the current working set has ended.

Oh, nice. That's a much better idea.

@dayoadeyemi
Copy link
Contributor Author

Yea if I don't index-- when the substream ends it will end up only allowing 1 process to run at a time. I've added that in and renamed index to processCount so that it is more descriptive.

*
* @id mergeLimit
* @section Higher-order Streams
* @name Stream.mergeWithLimit()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Call this @name Stream.mergeWithLimit(n).

The @id field should be @id mergeWithLimit.

You also need to add a @param n field for the parameter.

@vqvu
Copy link
Collaborator

vqvu commented Sep 16, 2015

I think this is ready as soon as you fix a few issues with the documentation.

@svozza
Copy link
Collaborator

svozza commented Sep 19, 2015

A couple of stray console.log debug statements commented out but other than that this looks good to merge (pardon the pun).

@vqvu
Copy link
Collaborator

vqvu commented Sep 20, 2015

Cool. Commented console.log are fine, since they're all over the rest of the codebase anyway.

Thanks, @dayoadeyemi for the PR!

vqvu added a commit that referenced this pull request Sep 20, 2015
Add mergeLimit function. Resolves #374.
@vqvu vqvu merged commit b891782 into caolan:master Sep 20, 2015
@vqvu vqvu added this to the v2.6.0 milestone Nov 18, 2015
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.

3 participants