search

dominictarr / through

simple way to create a ReadableWritable stream that works
Other
669 stars 64 forks source link

Consistency with return values #21

Closed chrisnojima closed 9 years ago

chrisnojima commented 10 years ago

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

dominictarr commented 10 years ago

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?

chrisnojima commented 10 years ago

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.

dominictarr commented 10 years ago

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.

chrisnojima commented 10 years ago

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.