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

readable.pipe returns stream.Readable #22341

Closed
andreasg123 opened this issue Aug 15, 2018 · 2 comments
Closed

readable.pipe returns stream.Readable #22341

andreasg123 opened this issue Aug 15, 2018 · 2 comments
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.

Comments

@andreasg123
Copy link
Contributor

readable.pipe is documented to return stream.Writable. However, as only stream.Readable has a pipe method, it only makes sense to return that to set up chains of piped streams. I also verified that a pipe to a Gunzip object could be used as a Readable stream.

@addaleax addaleax added question Issues that look for answers. stream Issues and PRs related to the stream subsystem. labels Aug 15, 2018
@addaleax
Copy link
Member

Hi! Can you explain what you are asking here? While everything you said is accurate, it’s not clear what you would like to see changed (if anything).

Chaining pipes using this method only makes sense if the target Writable also implements the Readable interface, yes. For Gunzip streams, or generally Duplex streams, that is the case.

If the returned value is not a Readable instance, then chaining pipes won’t work, but it’s still a meaningful thing and may be helpful to have (e.g. event listeners can be installed on it).

I think changing what .pipe() returns would be too big of a breaking change, in case that’s what you are suggesting here.

@andreasg123
Copy link
Contributor Author

I'm only asking to make the documentation a little clearer. I guess I should have checked the code to determine that the destination parameter to pipe is just the return value (currently not documented). I think that it would be beneficial to document explicitly that chaining pipes only works if the target Writable also implements the Readable interface. In the current documentation, it is confusing to understand why returning a Writable would support chaining.

It would also be helpful to document that Gunzip implements both Readable and Writable. That's kind of implied but there is no documentation on the methods of this class or its siblings.

@addaleax addaleax added doc Issues and PRs related to the documentations. and removed question Issues that look for answers. labels Aug 15, 2018
targos pushed a commit that referenced this issue Aug 19, 2018
Document how pipes can be chained in readable.pipe().
Document that zlib.Zlib inherits from stream.Transform.

PR-URL: #22354
Fixes: #22341
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
Document how pipes can be chained in readable.pipe().
Document that zlib.Zlib inherits from stream.Transform.

PR-URL: #22354
Fixes: #22341
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants