support drop(), issue #11 #75

alexbeletsky · 2014-05-04T17:33:37Z

Provides support for .drop()

caolan · 2014-05-12T14:33:49Z

lib/index.js

+    return this.consume(function (err, x, push, next) {
+        if (err) {
+            push(err);
+            if (n < 0) {


Should this be n >= 0 ?

caolan · 2014-05-12T14:34:30Z

Thanks for this, looks good apart from the comment I added. You might want to add a test for error handling to make sure it does what's expected here.

alexbeletsky · 2014-05-12T14:36:38Z

@caolan thanks! I'm wondering how to trigger err during consuming.. are there some tests that do anything similar?

Will extend test cases and re-apply.

caolan · 2014-05-12T14:38:57Z

@alexanderbeletsky you'll have to use a generator stream and push(error) onto it to cause the downstream .drop() to consume them. See the .errors() and .stopOnError() tests for examples: https://github.com/caolan/highland/blob/master/test/test.js#L408-L427

alexbeletsky · 2014-05-19T09:38:09Z

@caolan will be happy if you take a look on updated version :)

vqvu · 2014-05-22T11:56:41Z

lib/index.js

+ * @id drop
+ * @section Streams
+ * @name Stream.drop(n)
+ * @param {Number} n - n - integer representing number of values to read from source


Duplicate n -.

vqvu · 2014-05-22T12:03:00Z

I'm a little concerned about the handling of errors.

The function seems to impose an unnecessary stopOnError, since it pushes nil once it see the first error that comes after the first n values.
All errors before the first n values are emitted. This seems to contradict the behavior of take, where no errors after the first n values are emitted. I would expect that only errors emitted after the first n values are allowed through.

alexbeletsky · 2014-05-23T07:29:51Z

@vqvu I think your concerns are right.. I just wasn't sure about right behavior. Would be happy to clarify.

The current behavior,

exports['drop - errors'] = function (test) {
    test.expect(3);
    var s = _(function (push, next) {
        push(null, 1),
        push(new Error('error'), 2),
        push(null, 3),
        push(null, 4),
        push(null, _.nil)
    });
    var f = s.drop(2);
    f.pull(function (err, x) {
        test.equal(err.message, 'error');
    });
    f.pull(function (err, x) {
        test.equal(x, 4);
    });
    f.pull(function (err, x) {
        test.equal(x, _.nil);
    });
    test.done();
};

Expected behaviour,

exports['drop - errors'] = function (test) {
    test.expect(3);
    var s = _(function (push, next) {
        push(null, 1),
        push(new Error('error'), 2),
        push(null, 3),
        push(null, 4),
        push(null, _.nil)
    });
    var f = s.drop(2);
    f.pull(function (err, x) {
        test.equal(x, 4);
    });
    f.pull(function (err, x) {
        test.equal(x, _.nil);
    });
    test.done();
};

exports['drop - errors 2'] = function (test) {
    test.expect(3);
    var s = _(function (push, next) {
        push(null, 1),
        push(null, 2),
        push(new Error('error'), 3),
        push(null, 4),
        push(null, _.nil)
    });
    var f = s.drop(2);
    f.pull(function (err, x) {
        test.equal(err.message, 'error');
    });
    f.pull(function (err, x) {
        test.equal(x, _.nil);
    });
    test.done();
};

?

caolan · 2014-05-23T14:24:48Z

I'm not sure. The reason take() doesn't return any errors after the first n elements is because the rest of the stream isn't evaluated. With drop() we have to evaluate the first n items in order to ignore them. If an evaluation results in an error then it should probably be passed along the stream regardless of whether a result in that position would be ignored... though I could potentially see the case for dropping them too, I think I'd rather err on the side of caution...

vqvu · 2014-05-23T17:22:12Z

@caolan Good point. It's probably better to be safe. I'm more used to the Rx way of handling errors, so I think I was thrown off by the "more than one error per stream" semantics that we have here.

@alexanderbeletsky I'm now OK with keeping the drop - error behavor as it currently is, but I think drop - error 2 should be:

exports['drop - errors 2'] = function (test) {
    test.expect(3);
    var s = _(function (push, next) {
        push(null, 1),
        push(null, 2),
        push(new Error('error'), 3),
        push(null, 4),
        push(null, _.nil)
    });
    var f = s.drop(2);
    f.pull(function (err, x) {
        test.equal(err.message, 'error');
    });
    f.pull(function (err, x) {
        test.equal(x, 4);
    });
    f.pull(function (err, x) {
        test.equal(x, _.nil);
    });
    test.done();
};

An error shouldn't cause the stream to stop, since no other stream operators do this.

caolan · 2014-05-27T09:29:23Z

"An error shouldn't cause the stream to stop" - correct :) - since it's lazy it's up to the consumer to decide whether to continue reading. Obviously, if the stream is a data source and the error is unrecoverable, then the next value will just be nil.

caolan · 2014-07-12T20:47:41Z

@vqvu @alexanderbeletsky where are we with this?

alexbeletsky · 2014-07-18T17:07:15Z

@caolan I need to correct that to latest discussed behaviour. Will try to re-submit asap.

apaleslimghost · 2014-11-22T08:56:44Z

@alexanderbeletsky what's the status on this? Current branch has merge conflicts so you might want to rebase on master.

alexbeletsky · 2014-11-24T10:33:10Z

@quarterto unfortunately, I have to admit I failed to understand how the error handling should work, so my progress just stopped.

I will try to rebase and re-submit so we can discuss it again.

jeromew · 2015-01-05T20:21:14Z

@alexbeletsky sorry for the delay on this.

regarding the question on how error handling should work : all errors should be passed along downstream.

that would mean replace

+    if (err) {
+            push(err);
+            if (n < 0) {
+                next();
+            }
+            else {
+                push(null, nil);
+            }
+        }

by

+       if (err) {
+            push(err);
+            next();
+        }

do you still have the motivation / available time to finish this PR or would you prefer if someone can take it over ?

alexbeletsky · 2015-01-08T09:02:51Z

Yes, I'm still willing to help and make this PR happen!

On Mon, Jan 5, 2015, 21:21 jeromew notifications@github.com wrote:

@alexbeletsky https://github.com/alexbeletsky sorry for the delay on
this.

regarding the question on how error handling should work : all errors
should be passed along downstream.

that would mean replace
if (err) {+ push(err);+ if (n < 0) {
           next();+            }+            else {+                push(null, nil);+            }+        }
by
  if (err) {+            push(err);
       next();+        }
do you still have the motivation / available time to finish this PR or
would you prefer if someone can take it over ?

—
Reply to this email directly or view it on GitHub
#75 (comment).

svozza · 2015-03-01T19:02:04Z

What's the status of this? Do we even need it now that we have transducers?

vqvu · 2015-03-01T23:52:33Z

We don't need it, but I think we should have it as a pair with take.

svozza · 2015-03-03T15:31:33Z

Yep, it makes sense to have the both of them.

@alexbeletsky There's not very much left on this to do so I grabbed the changes from your pull request using the hub tool and I can make the minor adjustments for you. Do you mind if I finish this pull request on your behalf? It's just it's been open for a while and it would be nice to close it.

alexbeletsky · 2015-03-03T15:59:22Z

@svozza please, go ahead! Thanks a lot for your help, if any question plz ping me.

svozza · 2015-03-03T16:23:47Z

No probs!

vqvu · 2015-03-05T13:42:53Z

Superseded by #244.

caolan reviewed May 12, 2014
View reviewed changes

lib/index.js

return this.consume(function (err, x, push, next) {

if (err) {

push(err);

if (n < 0) {

Copy link

Owner

caolan May 12, 2014

Choose a reason for hiding this comment

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

Should this be n >= 0 ?

greelgorke mentioned this pull request May 21, 2014

added the rest function #85

Merged

vqvu reviewed May 22, 2014
View reviewed changes

alexbeletsky added 4 commits November 24, 2014 11:36

support drop(), issue caolan#11

be91675

corrected function @id

2963b28

corrected after review + added drop() errors handling test case

5c1a599

removed double n

1f9556c

alexbeletsky force-pushed the drop branch from 4ef7f34 to 1f9556c Compare November 24, 2014 10:37

svozza mentioned this pull request Mar 3, 2015

Tidy up support for drop() PR #244

Merged

vqvu closed this Mar 5, 2015

svozza mentioned this pull request Apr 9, 2015

New Release #267

Closed

vqvu added this to the v2.5.0 milestone Apr 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support drop(), issue #11 #75

support drop(), issue #11 #75

alexbeletsky commented May 4, 2014

caolan May 12, 2014

caolan commented May 12, 2014

alexbeletsky commented May 12, 2014

caolan commented May 12, 2014

alexbeletsky commented May 19, 2014

vqvu May 22, 2014

vqvu commented May 22, 2014

alexbeletsky commented May 23, 2014

caolan commented May 23, 2014

vqvu commented May 23, 2014

caolan commented May 27, 2014

caolan commented Jul 12, 2014

alexbeletsky commented Jul 18, 2014

apaleslimghost commented Nov 22, 2014

alexbeletsky commented Nov 24, 2014

jeromew commented Jan 5, 2015

alexbeletsky commented Jan 8, 2015

svozza commented Mar 1, 2015

vqvu commented Mar 1, 2015

svozza commented Mar 3, 2015

alexbeletsky commented Mar 3, 2015

svozza commented Mar 3, 2015

vqvu commented Mar 5, 2015

support drop(), issue #11 #75

support drop(), issue #11 #75

Conversation

alexbeletsky commented May 4, 2014

caolan May 12, 2014

Choose a reason for hiding this comment

caolan commented May 12, 2014

alexbeletsky commented May 12, 2014

caolan commented May 12, 2014

alexbeletsky commented May 19, 2014

vqvu May 22, 2014

Choose a reason for hiding this comment

vqvu commented May 22, 2014

alexbeletsky commented May 23, 2014

caolan commented May 23, 2014

vqvu commented May 23, 2014

caolan commented May 27, 2014

caolan commented Jul 12, 2014

alexbeletsky commented Jul 18, 2014

apaleslimghost commented Nov 22, 2014

alexbeletsky commented Nov 24, 2014

jeromew commented Jan 5, 2015

alexbeletsky commented Jan 8, 2015

svozza commented Mar 1, 2015

vqvu commented Mar 1, 2015

svozza commented Mar 3, 2015

alexbeletsky commented Mar 3, 2015

svozza commented Mar 3, 2015

vqvu commented Mar 5, 2015