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

seppevs · 2017-02-06T09:59:45Z

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

jasnell · 2017-02-06T15:08:58Z

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 · 2017-02-06T15:13:51Z

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 · 2017-02-06T15:25:05Z

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

Fishrock123 · 2017-02-06T15:32:49Z

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 · 2017-02-06T15:34:44Z

That would be certainly more useful

sam-github · 2017-02-11T20:08:01Z

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 · 2017-02-11T20:09:52Z

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

seppevs · 2017-02-13T09:53:08Z

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

sam-github

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

sam-github · 2017-02-16T19:29:43Z

doc/api/process.md

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!

sam-github · 2017-02-16T19:29:55Z

doc/api/process.md

"(which inherits"

sam-github · 2017-02-16T20:12:42Z

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

jasnell

Almost there, small nit

jasnell · 2017-02-16T20:50:25Z

doc/api/process.md

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

seppevs · 2017-02-17T08:16:09Z

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

Fishrock123

LGTM with one small comment

Fishrock123 · 2017-02-17T20:40:46Z

doc/api/process.md

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

@Fishrock123 done

fhinkel · 2017-03-26T10:13:16Z

@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

seppevs · 2017-03-27T07:40:05Z

@fhinkel done!

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes#9201 PR-URL: #11194 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

aqrln · 2017-03-27T14:16:49Z

@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 · 2017-03-27T14:21:32Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

MylesBorins · 2017-04-18T22:58:05Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixesnodejs/node#9201 PR-URL: nodejs/node#11194 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

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 5a00980Compare 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 a7b5e61Compare 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

doc/api/process.md Outdated
Copy link
Member
jasnellFeb 16, 2017
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 force-pushed the docs_stdout_stderr_and_stdin_are_duplex_streams branch from a7b5e61 to d5793b7Compare 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 a2f9c82Compare 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 b3a6a25Compare 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

Uh oh!

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

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

Uh oh!

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-githubFeb 16, 2017

Choose a reason for hiding this comment

Uh oh!

seppevsFeb 16, 2017

Choose a reason for hiding this comment

Uh oh!

sam-githubFeb 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!

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

Fishrock123Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

seppevsFeb 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

Fishrock123 commented Feb 6, 2017•
edited
Loading