Skip to content

lib: update outdated comment#8988

Closed
tanujasawant wants to merge 2 commits intonodejs:masterfrom
tanujasawant:master
Closed

lib: update outdated comment#8988
tanujasawant wants to merge 2 commits intonodejs:masterfrom
tanujasawant:master

Conversation

@tanujasawant
Copy link
Copy Markdown
Contributor

Checklist
  • commit message follows commit guidelines
Affected core subsystem(s)

comments

Description of change

Updated an outdated comment in child_process.js to describe the code better.

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Oct 9, 2016
@tanujasawant
Copy link
Copy Markdown
Contributor Author

from #8968

if (!Array.isArray(options.stdio)) {
// Leave stdin open for the IPC channel. stdout and stderr should be the
// same as the parent's if silent isn't set.
// Use a separate fd=3 for IPC channel. Inherit stdin, stdout, and stderr
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please leave "the" in front of "IPC channel".

Copy link
Copy Markdown
Contributor Author

@tanujasawant tanujasawant Oct 9, 2016

Choose a reason for hiding this comment

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

Umm I have left out the 'the'
@cjihrig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I mean please leave it as it was. In other words, have it say "for the IPC channel."

Copy link
Copy Markdown
Contributor Author

@tanujasawant tanujasawant Oct 9, 2016

Choose a reason for hiding this comment

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

Oh okay, thanks. It's done =)

Copy link
Copy Markdown
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.

LGTM

@tanujasawant
Copy link
Copy Markdown
Contributor Author

@jasnell, when will this commit be merged into nodejs:master ?

jasnell pushed a commit that referenced this pull request Oct 14, 2016
PR-URL: #8988
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Oct 14, 2016

Merged! Landed in 835fbee! Thank you @Tanuja-Sawant !

@jasnell jasnell closed this Oct 14, 2016
jasnell pushed a commit that referenced this pull request Oct 14, 2016
PR-URL: #8988
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
PR-URL: #8988
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_process Issues and PRs related to the child_process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants