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

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

Conversation

seppevs
Copy link
Contributor

@seppevs seppevs commented Feb 6, 2017

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

@nodejs-github-bot 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
@jasnell
Copy link
Member

jasnell commented Feb 6, 2017

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.

@addaleax
Copy link
Member

addaleax commented Feb 6, 2017

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.

@jasnell
Copy link
Member

jasnell commented Feb 6, 2017

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

@Fishrock123
Copy link
Contributor

Fishrock123 commented Feb 6, 2017

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.

@jasnell
Copy link
Member

jasnell commented Feb 6, 2017

That would be certainly more useful

@sam-github
Copy link
Contributor

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.

@sam-github
Copy link
Contributor

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

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

seppevs commented Feb 13, 2017

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

@Fishrock123 Fishrock123 self-assigned this Feb 14, 2017
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

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

associated with `stderr` (fd `2`).
The `process.stderr` property returns a stream equivalent to or
associated with `stderr` (fd `2`). It is a `net.Socket` (inherits from `stream.Duplex`)
unless it is from/to a file, in which case it is a [Writable][] stream.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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!

The `process.stderr` property returns a [Writable][] stream equivalent to or
associated with `stderr` (fd `2`).
The `process.stderr` property returns a stream equivalent to or
associated with `stderr` (fd `2`). It is a `net.Socket` (inherits from `stream.Duplex`)
Copy link
Contributor

Choose a reason for hiding this comment

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

"(which inherits"

@seppevs 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
Copy link
Contributor

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Almost there, small nit

`stderr` (fd `2`).
The `process.stderr` property returns a stream connected to
`stderr` (fd `2`). It is a `net.Socket` (which inherits from
`stream.Duplex`) unless fd `2` refers to a file, in which case it is
Copy link
Member

Choose a reason for hiding this comment

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

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

@seppevs seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from a7b5e61 to d5793b7 Compare February 17, 2017 08:15
@seppevs
Copy link
Contributor Author

seppevs commented Feb 17, 2017

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

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

LGTM with one small comment

The `process.stderr` property returns a [Writable][] stream connected to
`stderr` (fd `2`).
The `process.stderr` property returns a stream connected to
`stderr` (fd `2`). It is a `net.Socket` (which is a [Duplex][]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seppevs seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from d5793b7 to a2f9c82 Compare February 22, 2017 09:39
@fhinkel
Copy link
Member

fhinkel commented Mar 26, 2017

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

@seppevs seppevs force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from a2f9c82 to b3a6a25 Compare March 27, 2017 07:39
@seppevs
Copy link
Contributor Author

seppevs commented Mar 27, 2017

@fhinkel done!

fhinkel pushed a commit that referenced this pull request Mar 27, 2017
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>
@aqrln
Copy link
Contributor

aqrln commented Mar 27, 2017

@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?

@Fishrock123
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
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>
@MylesBorins
Copy link
Contributor

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

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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>
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. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants