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

Closed
wants to merge 1 commit into from
Closed

lib: update outdated comment #8968

wants to merge 1 commit into from

Conversation

tanujasawant
Copy link
Contributor

@tanujasawant tanujasawant commented Oct 7, 2016

Checklist
Affected core subsystem(s)

just 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 7, 2016
@tanujasawant tanujasawant changed the title Changed the comment on lines 53-54, fixing issue 8927 lib: update outdated comment Oct 7, 2016
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.
Thank you @Tanuja-Sawant!
This change is small enough that it should not need to wait the full 48 hours to land.

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

One nit: @Tanuja-Sawant, can you please update the commit log message to follow the guidelines described in the contributor's guide?

@tanujasawant
Copy link
Contributor Author

@jasnell is this commit log fine?

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

@Tanuja-Sawant ... my apologies for not being clear. I meant the actual commit log message here: e5ed61f. You can update it using git rebase -i HEAD~1 on your local branch, rewording it, then force pushing the updated commit back to your github branch. An alternative method is shown here: https://help.github.com/articles/changing-a-commit-message/.

@@ -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=4 for IPC channel. Inherit stdin, stdout and stderr
Copy link
Member

Choose a reason for hiding this comment

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

I think the correct fd index for the IPC channel here should be fd=3, not fd=4.

Quoting the docs:
The fds 0, 1, and 2 correspond to stdin, stdout, and stderr, respectively.

@@ -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=4 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 "the" before "IPC channel". Also, please add a comma after "stdout."

@tanujasawant
Copy link
Contributor Author

Hi!
I made a new pull request #8988

@tanujasawant tanujasawant mentioned this pull request Oct 9, 2016
1 task
@mscdex mscdex closed this Oct 9, 2016
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.

6 participants