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

implement <name> and <name>Series with <name>Limit #847

Merged
merged 1 commit into from
Mar 5, 2016
Merged

implement <name> and <name>Series with <name>Limit #847

merged 1 commit into from
Mar 5, 2016

Conversation

charlierudolph
Copy link
Contributor

@aearly @megawac

fixes #778

Ended up adding concatLimit and detectLimit in the process.
Still need to add tests and documentation

@megawac
Copy link
Collaborator

megawac commented Jul 16, 2015

Cool! Have you run benchmark against master?

./perf/benchmark.js master

callback(null);
if (r.length >= a.length) {
if (done) {
test.same(r, a);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats the point of these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test fail without them. With how eachOfLimit is executed since the first iteration is synchronous, it executed the second iteration and so on so this causes multiple calls to test.done()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, alright that's breaking and should be fixed in v2

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've created a 2.x branch we could merge this PR into. Although, I'm a bit hesitant because it could set us up for merge hell if we end up making lots of changes on master.

@megawac
Copy link
Collaborator

megawac commented Jul 16, 2015

.As one might expect, the results of running benchmark indicate a significant his (on the order of several ms in some cases) to parallel functions and a minor hit to series functions.

I think this is fine for series, but parallel cases will require some optimization. Perhaps adding a special case to eachOfLimit for Infinitity?

Benchmark results: https://gist.github.com/megawac/b5e4120f04f1c7712639

@charlierudolph
Copy link
Contributor Author

The parallel case doesn't look any better inside eachOfLimit so I'd suggest leaving it as is in the current codebase. I can change this to simply implement <name>Series with <name>Limit.

@megawac
Copy link
Collaborator

megawac commented Jul 17, 2015

I'll look into this further over the weekend, but I'd be surprised if eachLimit couldn't be adapted for Infinity in a relatively clean way

@megawac
Copy link
Collaborator

megawac commented Jul 17, 2015

I do really like this direction though as it will make generators and iterators much easier to support

This was referenced Jul 19, 2015
@megawac megawac mentioned this pull request Jul 31, 2015
@mmiller42 mmiller42 mentioned this pull request Dec 5, 2015
@charlierudolph
Copy link
Contributor Author

@aearly @megawac any update this on this?

@aearly
Copy link
Collaborator

aearly commented Feb 28, 2016

Master has change significantly with the modularization. We ready for this change, though. 👍

@charlierudolph
Copy link
Contributor Author

@aearly updated

@megawac
Copy link
Collaborator

megawac commented Mar 5, 2016

👍 after review. Feel free to merge alex if you're happy with it

aearly added a commit that referenced this pull request Mar 5, 2016
implement <name> and <name>Series with <name>Limit
@aearly aearly merged commit f38483c into caolan:master Mar 5, 2016
@aearly
Copy link
Collaborator

aearly commented Mar 5, 2016

I love seeing code changes that remove 60 lines of code without taking away functionality. 😄

@aearly aearly added this to the 2.0 milestone Mar 12, 2016
@elmigranto elmigranto mentioned this pull request Jun 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible internal interface update
3 participants