doc: fixes default shell in child_process.md #14203

henryzxu · 2017-07-12T19:30:19Z

This PR clarifies that 'process.env.ComSpec' is the default shell in
Windows and that 'cmd.exe' is only used as a fallback.

Fixes: #14156

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Clarifies the default shell in Windows is process.env.ComSpec and that cmd.exe is only used as a fallback. Functions whose descriptions are affected include: child_process.spawn, child_process.exec, child_process.spawnsSync, and child_process.execSync. Fixes: nodejs#14156

Clarifies that the default shell in Windows is process.env.ComSpec and that cmd.exe is only used as a fallback. Functions whose descriptions are affected include: child_process.spawn, child_process.exec, child_process.spawnSync, and child_process.execSync. Fixes: nodejs#14156

vsemozhetbyt · 2017-07-12T20:08:58Z

Thank you!

A nit: the length of some new lines is more than 80 chars now. Can you rewrap them according to our style guide?

Rewrapped lines over 80 characters according to the Style Guide.

henryzxu · 2017-07-12T20:29:11Z

No problem! I rewrapped the lines in my latest commit.

Thanks for the feedback!

tniessen

I am kind of unsatisfied with having the same content four times, even though it is not too long... Still not pretty IMO.

henryzxu · 2017-07-12T23:13:55Z

Would a footnote be more appropriate in this situation?

tniessen · 2017-07-12T23:43:12Z

I would prefer that, maybe a small section at the bottom of the page (similar to this section)? Let's wait for another opinion on this :)

henryzxu · 2017-07-13T00:01:18Z

I'll see what I can do once we get another opinion! I do like how much cleaner that solution is compared to my current commit.

tniessen · 2017-07-16T15:12:30Z

@nodejs/documentation opionions regarding the current state vs. a footnote / paragraph at the bottom?

benjamingr · 2017-07-16T15:17:33Z

+1 for footnote - I think adding too much text to parameter definitions can be really confusing.

Reorganizes `child_process` shell spawning information. Fixes: nodejs#14156

henryzxu · 2017-07-17T18:01:04Z

I split the shell spawn information into two sections at the bottom, Shell Requirements and Default Windows Shell. I was having a bit of trouble combining the information into one section logically, so if you have any suggestions, let me know!

tniessen · 2017-07-17T18:01:48Z

doc/api/child_process.md

-    to `false` (no shell).
+    `'/bin/sh'` on UNIX, and `'process.env.ComSpec'` on Windows. See 
+    [Shell Requirements][] and [Default Windows Shell][]. A different shell can 
+    be specified as a string. Defaults to `false` (no shell).


Any reason why you swapped the last two sentences compared to spawn?

Oh oops! That was an accident. It should be fixed in the latest commit.

vsemozhetbyt · 2017-07-17T18:07:57Z

doc/api/child_process.md

@@ -1303,6 +1314,8 @@ to `stdout` although there are only 4 characters.
 [`child_process.spawn()`]: #child_process_child_process_spawn_command_args_options
 [`child_process.spawnSync()`]: #child_process_child_process_spawnsync_command_args_options
 [`maxBuffer` and Unicode]: #child_process_maxbuffer_and_unicode
+[Shell Requirements]: #child_process_shell_requirements


This and the next references are out of ASCII sort order (should be placed before the last reference).

Thanks for the heads up! They should be in the correct order in the latest commit.

Small formatting fixes for shell spawning information. Fixes: nodejs#14156

henryzxu · 2017-07-17T18:47:25Z

The latest commit should be updated with your reviews. Let me know if you have any more comments or suggestions! Thanks for making my first pull request a very enjoyable experience!

tniessen

Thank you for your efforts! 😃 I added some final nits but everything else looks great!

Don't worry, it is completely normal that non-trivial changes need to be reviewed and adapted several times. This is our way to maintain and improve the quality of our project :)

tniessen · 2017-07-17T22:07:55Z

doc/api/child_process.md

-    (Default: `'/bin/sh'` on UNIX, `'cmd.exe'` on Windows, The shell should
-     understand the `-c` switch on UNIX or `/d /s /c` on Windows. On Windows,
-     command line parsing should be compatible with `cmd.exe`.)
+    (Default: `'/bin/sh'` on UNIX, `'process.env.ComSpec'` on Windows. See 


process.env.ComSpec is a property (variable), not a string literal, so it should not be quoted.

tniessen · 2017-07-17T22:08:06Z

doc/api/child_process.md

-    `'/bin/sh'` on UNIX, and `'cmd.exe'` on Windows. A different shell can be
-    specified as a string. The shell should understand the `-c` switch on UNIX,
-    or `/d /s /c` on Windows. Defaults to `false` (no shell).
+    `'/bin/sh'` on UNIX, and `'process.env.ComSpec'` on Windows. A different 


tniessen · 2017-07-17T22:08:14Z

doc/api/child_process.md

-    (Default: `'/bin/sh'` on UNIX, `'cmd.exe'` on Windows, The shell should
-     understand the `-c` switch on UNIX or `/d /s /c` on Windows. On Windows,
-     command line parsing should be compatible with `cmd.exe`.)
+    (Default: `'/bin/sh'` on UNIX, `'process.env.ComSpec'` on Windows. See 


tniessen · 2017-07-17T22:08:26Z

doc/api/child_process.md

-    `'/bin/sh'` on UNIX, and `'cmd.exe'` on Windows. A different shell can be
-    specified as a string. The shell should understand the `-c` switch on UNIX,
-    or `/d /s /c` on Windows. Defaults to `false` (no shell).
+    `'/bin/sh'` on UNIX, and `'process.env.ComSpec'` on Windows. A different 


tniessen · 2017-07-17T22:08:39Z

doc/api/child_process.md

+Although Microsoft specifies `'process.env.ComSpec'` must contain the path to
+`'cmd.exe'` in the root environment, child processes are not always subject to
+the same requirement. Thus, in `child_process` functions where a shell can be 
+spawned, `'cmd.exe'` is used as a fallback if `'process.env.ComSpec'` is 


tniessen · 2017-07-17T22:10:23Z

doc/api/child_process.md

+
+## Default Windows Shell
+
+Although Microsoft specifies `'process.env.ComSpec'` must contain the path to


Microsoft does not specify anything regarding process.env.ComSpec. This is just node's representation of the process environment. Actually, Microsoft only holds that assertion for the environment variable as stored in the registry. The technical background will be too complex for our docs and is not really relevant, but I would prefer %COMSPEC% instead of process.env.ComSpec here (and only here).

Notation changes for `process.env.ComSpec` Fixes: nodejs#14156

henryzxu · 2017-07-18T16:23:42Z

The references to process.env.ComSpec should all be fixed!

Thank you for all your help! The level of detail you put into your feedback makes me look forward to contributing in the future! 😃

tniessen

Last nit, I swear!

tniessen · 2017-07-18T17:36:23Z

doc/api/child_process.md

+
+## Default Windows Shell
+
+Although Microsoft specifies `%COMSPEC` must contain the path to


Environment variables must be enclosed in %s under Windows, so it should be %COMSPEC% (note the % at the end).

Wrapped `%COMSPEC%` with percent signs. Fixes: nodejs#14156

henryzxu · 2017-07-18T17:47:13Z

Oh oops! It should be fixed now. :)

Clarifies the default shell in Windows is process.env.ComSpec and that cmd.exe is only used as a fallback. Functions whose descriptions are affected include: child_process.spawn, child_process.exec, child_process.spawnsSync, and child_process.execSync. PR-URL: #14203 Fixes: #14156 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

tniessen · 2017-07-20T13:11:55Z

Landed in 7fdcb68 🎉 Thank you very much! 😃

A few tips for future contributions:

You might want to disable BOM (byte-order mark) in your editor when working on node, all of our files are encoded as UTF-8 without BOM (that's why GitHub says you changed the first line of the file).
You might also want to check your changes for trailing whitespace (e.g on this line). Even though we can fix these problems while landing PRs, it is a good habit to pay attention to that.
You don't need to use the same commit title for fixups (follow-up commits). Actually, it is usually easier for us to keep track of your commits if you include a summary of the new changes in the commit title. The first commit of the PR should use the format as described in CONTRIBUTING.md though :)

henryzxu · 2017-07-20T19:29:56Z

Wow, my pull request landing is more exciting than I could've imagined! Thank you so much for all your help! 😃

Thank you also for the helpful tips! They answered a number of questions that I didn't quite know how to ask. :)

I look forward to contributing again soon!

Clarifies the default shell in Windows is process.env.ComSpec and that cmd.exe is only used as a fallback. Functions whose descriptions are affected include: child_process.spawn, child_process.exec, child_process.spawnsSync, and child_process.execSync. PR-URL: #14203 Fixes: #14156 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

MylesBorins · 2017-08-16T07:44:02Z

is this accurate for v6.x as well?

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jul 12, 2017

henryzxu force-pushed the my-branch branch from 0adc489 to e9089d3 Compare July 12, 2017 19:57

vsemozhetbyt approved these changes Jul 12, 2017

View reviewed changes

doc: fixes default shell in child_process.md

a6a073c

Rewrapped lines over 80 characters according to the Style Guide.

tniessen reviewed Jul 12, 2017

View reviewed changes

jasnell approved these changes Jul 14, 2017

View reviewed changes

tniessen self-assigned this Jul 16, 2017

doc: fixes default shell in child_process.md

fd92d17

Reorganizes `child_process` shell spawning information. Fixes: nodejs#14156

tniessen reviewed Jul 17, 2017

View reviewed changes

vsemozhetbyt reviewed Jul 17, 2017

View reviewed changes

doc: fixes default shell in child_process.md

33adbeb

Small formatting fixes for shell spawning information. Fixes: nodejs#14156

tniessen reviewed Jul 17, 2017

View reviewed changes

doc: fixes default shell in child_process.md

e4171c3

Notation changes for `process.env.ComSpec` Fixes: nodejs#14156

tniessen reviewed Jul 18, 2017

View reviewed changes

doc: fixes default shell in child_process.md

46a8829

Wrapped `%COMSPEC%` with percent signs. Fixes: nodejs#14156

tniessen approved these changes Jul 20, 2017

View reviewed changes

tniessen closed this Jul 20, 2017

addaleax mentioned this pull request Jul 24, 2017

v8.3.0 proposal #14452

Closed

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Dec 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: fixes default shell in child_process.md #14203

doc: fixes default shell in child_process.md #14203

henryzxu commented Jul 12, 2017

vsemozhetbyt commented Jul 12, 2017

henryzxu commented Jul 12, 2017

tniessen left a comment

henryzxu commented Jul 12, 2017

tniessen commented Jul 12, 2017

henryzxu commented Jul 13, 2017

tniessen commented Jul 16, 2017

benjamingr commented Jul 16, 2017

henryzxu commented Jul 17, 2017

tniessen Jul 17, 2017

henryzxu Jul 17, 2017

vsemozhetbyt Jul 17, 2017 •

edited

Loading

henryzxu Jul 17, 2017

henryzxu commented Jul 17, 2017

tniessen left a comment •

edited

Loading

tniessen Jul 17, 2017

tniessen Jul 17, 2017

tniessen Jul 17, 2017

tniessen Jul 17, 2017

tniessen Jul 17, 2017

tniessen Jul 17, 2017

henryzxu commented Jul 18, 2017

tniessen left a comment

tniessen Jul 18, 2017 •

edited

Loading

henryzxu commented Jul 18, 2017

tniessen commented Jul 20, 2017

henryzxu commented Jul 20, 2017

MylesBorins commented Aug 16, 2017


		## Default Windows Shell

		Although Microsoft specifies `'process.env.ComSpec'` must contain the path to


		## Default Windows Shell

		Although Microsoft specifies `%COMSPEC` must contain the path to

doc: fixes default shell in child_process.md #14203

doc: fixes default shell in child_process.md #14203

Conversation

henryzxu commented Jul 12, 2017

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Jul 12, 2017

henryzxu commented Jul 12, 2017

tniessen left a comment

Choose a reason for hiding this comment

henryzxu commented Jul 12, 2017

tniessen commented Jul 12, 2017

henryzxu commented Jul 13, 2017

tniessen commented Jul 16, 2017

benjamingr commented Jul 16, 2017

henryzxu commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryzxu commented Jul 17, 2017

tniessen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryzxu commented Jul 18, 2017

tniessen left a comment

Choose a reason for hiding this comment

tniessen Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

henryzxu commented Jul 18, 2017

tniessen commented Jul 20, 2017

henryzxu commented Jul 20, 2017

MylesBorins commented Aug 16, 2017

vsemozhetbyt Jul 17, 2017 •

edited

Loading

tniessen left a comment •

edited

Loading

tniessen Jul 18, 2017 •

edited

Loading