Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Consistency with return values #21

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

Hi,
Just started looking at this library and noticed chaining is inconsistently applied with the return values. For example if I do (this contrived example)

tr.pause().resume().pause();
this will work

if i do
tr.pause().pause().resume()
this will not work

Owner

dominictarr commented Feb 4, 2014

This seems reasonable, but why do you want to do this?
can you describe the situation where this is a problem?
what are you trying to do?

This is just something I noticed while browsing the code and isn't really fixing an issue I am dealing with. I agree the use-case is probably rare, but it could lead to some surprising behavior. Probably best to fully support chaining or to not at all.

Owner

dominictarr commented Feb 5, 2014

Right - so this module is 2 years old, and no body has ever complained about this until now,
and it's used in hundreds of other modules. I approve of chaining on pause and resume, sometimes I do this:

a.pipe(b.pause())

I feel different about .end() because I can't see any reason to chain on ending a stream...
because the stream is over now.

I agree. I wasn't trying to make a judgement about which calls should allow chaining per se. I just noticed that end() does support chaining when you first call it, but not after. So if you (for some reason) did
a.end().pause() that will work but if a was already ended somewhere else, calling a.end().pause() will dereference null and crash.

Because this hasn't been a problem for the hundreds of users I assume they're really not using the chaining capabilities, or if they are it's in a very straightforward manner which doesn't cause the context to break themselves.

Since the code is so old and this isn't really fixing an outstanding issue feel free to ignore the changes. I was just perusing the code and noticed that could potentially be a gotcha. Thanks for taking the time to review the pull.

@chrisnojima chrisnojima closed this Aug 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment