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: update subprocess.killed #16748

Merged
merged 1 commit into from Nov 7, 2017
Merged

doc: update subprocess.killed #16748

merged 1 commit into from Nov 7, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 4, 2017

This commit changes the wording of subprocess.killed to reflect that a child process was successfully signaled, and not necessarily terminated.

Fixes: #16747

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@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 Nov 4, 2017
@@ -1044,10 +1044,10 @@ added: v0.5.10
-->

* {boolean} Set to `true` after `subprocess.kill()` is used to successfully
terminate the child process.
signal the child process.
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed : successfully signal -> successfully send a signal to?

Copy link

Choose a reason for hiding this comment

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

Up to me, proposal of @Trott is much better


The `subprocess.killed` property indicates whether the child process was
successfully terminated using `subprocess.kill()`.
successfully signaled using `subprocess.kill()`.
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed: was successfully signaled using -> successfully received a signal from?

@eduardbme
Copy link
Contributor

Maybe it's worth to explicitly point out that even in the case of cp.killed === true child process may still be alive, so don't rely on this param if you want to be sure that process was actually killed.

Some developers may not distinguish 'terminated' and 'signaled' terms correctly, so additional explanation will be helpful. After all it's a sign of a good documentation.

@cjihrig cjihrig force-pushed the kill-docs branch 2 times, most recently from f5ec3d4 to 8db8c2c Compare November 7, 2017 18:44
@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 7, 2017

This commit changes the wording of subprocess.killed to reflect
that a child process was successfully signaled, and not
necessarily terminated.

Fixes: nodejs#16747
PR-URL: nodejs#16748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig merged commit 2fdb606 into nodejs:master Nov 7, 2017
@cjihrig cjihrig deleted the kill-docs branch November 7, 2017 20:38
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
This commit changes the wording of subprocess.killed to reflect
that a child process was successfully signaled, and not
necessarily terminated.

Fixes: #16747
PR-URL: #16748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
This commit changes the wording of subprocess.killed to reflect
that a child process was successfully signaled, and not
necessarily terminated.

Fixes: #16747
PR-URL: #16748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
This commit changes the wording of subprocess.killed to reflect
that a child process was successfully signaled, and not
necessarily terminated.

Fixes: #16747
PR-URL: #16748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
This commit changes the wording of subprocess.killed to reflect
that a child process was successfully signaled, and not
necessarily terminated.

Fixes: #16747
PR-URL: #16748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
This commit changes the wording of subprocess.killed to reflect
that a child process was successfully signaled, and not
necessarily terminated.

Fixes: #16747
PR-URL: #16748
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants