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

lib: update outdated comment #8988

Closed
wants to merge 2 commits into from
Closed

lib: update outdated comment #8988

wants to merge 2 commits into from

Conversation

tanujasawant
Copy link
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
Contributor Author

from #8968

@@ -50,8 +50,8 @@ exports.fork = function(modulePath /*, args, options*/) {
args = execArgv.concat([modulePath], args);

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