FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: update outdated comment by tanujasawant · Pull Request #8988 · nodejs/node · GitHub

/ node Public

lib: update outdated comment - #8988

Closed
tanujasawant wants to merge 2 commits into
nodejs:masterfrom
tanujasawant:master
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

tanujasawant commented Oct 9, 2016

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 added the child_process Issues and PRs related to the child_process subsystem. label Oct 9, 2016
tanujasawant mentioned this pull request Oct 9, 2016
Closed

tanujasawant commented Oct 9, 2016

Copy link
Copy Markdown
Contributor Author

from #8968

tanujasawant mentioned this pull request Oct 9, 2016
Closed
cjihrig reviewed Oct 9, 2016
View reviewed changes
Comment thread lib/child_process.js Outdated
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

cjihrig Oct 9, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tanujasawant Oct 9, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Umm I have left out the 'the'
@cjihrig

cjihrig Oct 9, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tanujasawant Oct 9, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh okay, thanks. It's done =)

jasnell approved these changes Oct 10, 2016
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

tanujasawant commented Oct 14, 2016

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
child_process: update outdated comment
835fbee 
PR-URL: #8988
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Oct 14, 2016

Copy link
Copy Markdown
Member

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

jasnell closed this Oct 14, 2016
jasnell pushed a commit that referenced this pull request Oct 14, 2016
child_process: update outdated comment
6eece77 
PR-URL: #8988
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
child_process: update outdated comment
cbfde3c 
PR-URL: #8988
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig left review comments
jasnell jasnell approved these changes

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL