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

doc: some fixes in child_process.md #10102

Closed
wants to merge 1 commit into from
Closed

doc: some fixes in child_process.md #10102

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Dec 3, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, child_process

Description of change
  1. assert.equal => assert.strictEqual.
  2. let => const for the variable that is not reassigned.
  3. Fix indentation and internal spaces in code bits.
  4. Stringify buffers in code examples to prevent erroneous raw buffer outputs.
  5. Fix a typo.

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Dec 3, 2016
@vsemozhetbyt vsemozhetbyt changed the title Child process.md doc: some fixes in child_process.md Dec 3, 2016
@@ -91,11 +91,11 @@ const spawn = require('child_process').spawn;
const bat = spawn('cmd.exe', ['/c', 'my.bat']);

bat.stdout.on('data', (data) => {
console.log(data);
console.log(`${data}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say, calling toString explicitly would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following this line. Should I change it as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

console.log('%s', data)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thefourtheye, @sam-github So which of these two variants should I apply?

Copy link
Contributor

Choose a reason for hiding this comment

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

what two variants?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. I think console.log('%s', data) is more idiomatic. Code using console.log() should use its facilities. console.log(data.toString()) just seems a cumbersome version. @thefourtheye, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I feel like data.toString() has clearer intent, but I'm good either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we have 3:1 (considering emoji) for data.toString().

stdio: [
0, // Use parents stdin for child
'pipe', // Pipe child's stdout to parent
fs.openSync('err.out', 'w') // Direct child's stderr to a file
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an fd leak, because the fd is never closed

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Dec 4, 2016

Choose a reason for hiding this comment

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

@thefourtheye, @sam-github I'm not sure how to fix this. It requires rewriting the previous paragraph. Maybe it could be fixed in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

+1 for another PR. The issue is already there, this is just altering indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok by me

@sam-github
Copy link
Contributor

LGTM, despite the unexplained aversion to format strings :-)

@vsemozhetbyt
Copy link
Contributor Author

ping @nodejs/documentation

@Qard
Copy link
Member

Qard commented Dec 9, 2016

LGTM also.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash into one commit with a bullet list of changes, I would describe this as "doc: modernize child_process example code".

@vsemozhetbyt
Copy link
Contributor Author

@sam-github I hope I've done it right... I don't know why the bot has added this dont-land-on-v7.x label after squashing, though.

fs.openSync('err.out', 'w') // Direct child's stderr to a file
]
stdio: [
0, // Use parents stdin for child
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: parent's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.
sam-github pushed a commit to sam-github/node that referenced this pull request Dec 12, 2016
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: nodejs#10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@sam-github
Copy link
Contributor

Landed in f418a22

@sam-github sam-github closed this Dec 12, 2016
@vsemozhetbyt vsemozhetbyt deleted the child_process.md branch December 12, 2016 15:58
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: #10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: nodejs#10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: nodejs#10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: #10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: nodejs#10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: nodejs#10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
1. equal => strictEqual.
2. let => const for the variable that is not reassigned.
3. fix spaces.
4. stringify erroneous raw buffer outputs.
5. fix a typo.

PR-URL: #10102
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants