Clarify that ProcessStartInfo.UseShellExecute does not mean bash #436
Conversation
| @@ -1466,6 +1466,8 @@ p.WaitForExit(); | |||
|
|
|||
| Setting this property to `false` enables you to redirect input, output, and error streams. | |||
|
|
|||
| This property has nothing to do with command shells (`bash`, `sh`) on Unix-like platforms. The term shell in the name here is a reference to the graphical shell on Windows. The word "shell" in this context means the same as "desktop", and lets users launch graphical applications or open documents. | |||
There was a problem hiding this comment.
I'd probably rewrite the first two sentences a bit:
This property isn't related to command shells (bash, sh) on Unix-like platforms. The term shell in the API name is a reference to the graphical shell on Windows.
There was a problem hiding this comment.
I'd just shorten it to something like "The word "shell" in this context [UseShellExecute] refers to a graphical shell (similarly as on Windows) rather than command shells (i.e. bash or sh), and lets users launch graphical applications or open documents.". It can still use sh/bash in certain scenarios so "nothing to do" is IMO too strong.
|
@TSlivede Is this still a change we want, given the discussions in https://github.com/dotnet/corefx/issues/24704? |
If you ask me: Yes, something like this is a change, that we absolutely want. Although the future of To clear things up one could possibly add a link to the |
The property name is very confusing on unix-like platforms. Anyone familiar with unix-like platforms will probably assume it means /bin/bash or /bin/sh. Clarify that it does not mean that shell and that it refers to the conecpt of shell on Windows, which is closer in meaning to "desktop". For an example of this tripping up users, see: dotnet/core#1857. For more on the cross-platform inconsistencies of UseShellExecute, see https://github.com/dotnet/corefx/issues/24704
There was a problem hiding this comment.
LGTM content-wise, @mairaw, @rpetrusha please check for wording and style
rpetrusha
dismissed
mairaw’s stale review
Requested changes were addressed.
|
Thanks for merging the fix! |
The property name is very confusing on unix-like platforms. Anyone familiar with unix-like platforms will probably assume it means
/bin/bashor/bin/sh. Clarify that it does not mean that shell and thatit refers to the conecpt of shell on Windows, which is closer in meaning to "desktop".
For an example of this tripping up users, see: dotnet/core#1857. For more on the cross-platform inconsistencies of
UseShellExecute,see https://github.com/dotnet/corefx/issues/24704To be honest, I am not sure this is a good change, but I can't find a good style guide or examples on how to best document the caveats on different platforms. Suggestions on improving this change are more than welcome!