doc: stdout, stderr and stdin are all Duplex streams by seppevs · Pull Request #11194 · nodejs/node · GitHub

doc: stdout, stderr and stdin are all Duplex streams but documentation states otherwise

This is a fix for #9201

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

I'm not convinced that we should document these as Duplex and it's not entirely clear if that's always going to be the case. For instance, it does not make sense to treat stdin as a Writable.

For instance, it does not make sense to treat stdin as a Writable.

Just for clarification, that stdin is a “readable” stream and stdout/stderr are “writable” streams is only a convention and not enforced by the OS in any way (at least on UNIX and I think it applies to Windows, too).

I don’t think it’s Node’s place to enforce these convention when the OS doesn’t.

True, but it's also not that practically useful unless there's an obscure use case I'm missing.

True, but it's also not that practically useful unless there's an obscure use case I'm missing.

Maybe we could start printing warnings to fd 0. 😛

The docs are still technically correct due to the weirdness that is the stdio impl.

I think changing it to note that it is net.Socket (inherits from stream.Duplex) unless it is from/to a file, in which case it is a fs.sync{Write|Read}Stream, could be more useful.

That would be certainly more useful

Just because it may not be used very often doesn't mean its not useful. Why should we have an opinion on this, particularly if its platform independent, and pre-existing? There are interesting use-cases, like gpg, where processes read and write from arbitrary fds, I like that such programs can be written in node, that its not just for webapps.

Agree with #11194 (comment), we should doc its actual class.

I've amended changes to the commit so the actual class is now documented.

I left some suggestions, I find the "from/to" confusing, but basically LGTM.

I suggest: "unless fd 2 refers to a file, "

@sam-github Thank you for reviewing. I made the requested changes and rebased with master. I had some merge conflicts on the same sections in the docs.
Can you review again? Thanks!

"(which inherits"

code looks good, @Fishrock123 assigned this to himself, so he'll need a chance to review

you should shorten your commit message, guidelines are here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

Almost there, small nit

It would be worthwhile to have this (and the other reference below) be a link to the Duplex doc

@jasnell: I made the changes, can you review again?

LGTM with one small comment

Could you link net.Socket to it's definition in net.md? Thanks!

@Fishrock123 done

@seppevs sorry this took so long, by now process.md has changed. Can you rebase and make [Duplex] look like the other entries? Thanks!

…n states otherwise This is a fix for nodejs#9201

@fhinkel done!

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

@cjihrig just for the record, this patch had been landed in 1005b1d before your approval.
@fhinkel was this an accidental push or you just forgot to close the PR?

Looks like she forgot to close it. Please re-open if this was in error.

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

backported to v6.x-staging. If it should be backed out lmk

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes nodejs/node#9201 PR-URL: nodejs/node#11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Feb 6, 2017

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from 30d6058 to 5a00980 Compare February 13, 2017 09:51

Fishrock123 self-assigned this Feb 14, 2017

sam-github approved these changes Feb 16, 2017

View reviewed changes

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch 2 times, most recently from b78f255 to a7b5e61 Compare February 16, 2017 20:00

sam-github approved these changes Feb 16, 2017

View reviewed changes

jasnell requested changes Feb 16, 2017

View reviewed changes

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from a7b5e61 to d5793b7 Compare February 17, 2017 08:15

jasnell approved these changes Feb 17, 2017

View reviewed changes

Fishrock123 approved these changes Feb 17, 2017

View reviewed changes

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from d5793b7 to a2f9c82 Compare February 22, 2017 09:39

doc: stdout, stderr and stdin are all Duplex streams but documentatio…

b3a6a25

…n states otherwise This is a fix for nodejs#9201

seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from a2f9c82 to b3a6a25 Compare March 27, 2017 07:39

fhinkel approved these changes Mar 27, 2017

View reviewed changes

cjihrig approved these changes Mar 27, 2017

View reviewed changes

Fishrock123 closed this Mar 27, 2017

MylesBorins mentioned this pull request Mar 28, 2017

V7.8.0 proposal #12104

Merged

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the lts-watch-v6.x label Apr 18, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

doc: stdout, stderr and stdin are all Duplex streams by seppevs · Pull Request #11194 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: stdout, stderr and stdin are all Duplex streams - #11194

doc: stdout, stderr and stdin are all Duplex streams#11194seppevs wants to merge 1 commit intonodejs:masternodejs/node:masterfrom seppevs:docs_stdout_stderr_and_stdin_are_duplex_streamsseppevs/node:docs_stdout_stderr_and_stdin_are_duplex_streamsCopy head branch name to clipboard

Conversation

seppevs commented Feb 6, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Feb 6, 2017

Uh oh!

addaleax commented Feb 6, 2017

Uh oh!

jasnell commented Feb 6, 2017

Uh oh!

Fishrock123 commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Feb 6, 2017

Uh oh!

sam-github commented Feb 11, 2017

Uh oh!

sam-github commented Feb 11, 2017

Uh oh!

seppevs commented Feb 13, 2017

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

seppevs Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github commented Feb 16, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

seppevs commented Feb 17, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

seppevs Feb 22, 2017

Choose a reason for hiding this comment

Uh oh!

fhinkel commented Mar 26, 2017

Uh oh!

seppevs commented Mar 27, 2017

Uh oh!

aqrln commented Mar 27, 2017

Uh oh!

Fishrock123 commented Mar 27, 2017

Uh oh!

MylesBorins commented Apr 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

doc: stdout, stderr and stdin are all Duplex streams#11194
seppevs wants to merge 1 commit into
nodejs:masterfrom
seppevs:docs_stdout_stderr_and_stdin_are_duplex_streams

Fishrock123 commented Feb 6, 2017 •
edited
Loading