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: add documentation for killed property of ChildProcess instance #14578

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 1, 2017

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 child_process

@Trott Trott added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Aug 1, 2017
@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 Aug 1, 2017
@refack
Copy link
Contributor

refack commented Aug 1, 2017

Do we really want to go there? Can we skip straight to child.terminated? (deprecate killed and alias to terminated)

@jasnell
Copy link
Member

jasnell commented Aug 1, 2017

I agree, it may be a good opportunity to improve the naming here.

@refack
Copy link
Contributor

refack commented Aug 1, 2017

looking at the code

  if (this._handle) {
    var err = this._handle.kill(signal);
    if (err === 0) {
      /* Success. */
      this.killed = true;
      return true;
    }

So child.signalled will probably even be more correct, since it will be true for child.kill('SIGUSR1')

@Trott
Copy link
Member Author

Trott commented Aug 1, 2017

Do we really want to go there? Can we skip straight to child.terminated? (deprecate killed and alias to terminated)

TL;DR: I'd be more inclined to change child in the docs to subprocess than trying to alias killed.

A few things to bear in mind:

  • child.kill() is already documented and isn't going anywhere. Is it worth the effort to avoid child.killed when child.kill() is pretty much unavoidable and in the docs? (Rhetorical question, but I also don't know the answer. Maybe it's "yes" but I'm not sure.)

  • If we're going to change it up, IMO it's a more user-friendly change if we change it to subprocess.kill() and subprocess.killed. At least then users don't have to alter their code that works just fine right now.

  • Even if we deprecate .killed and add an alias, we still need to document .killed. It's in use in the ecosystem.

I'm happy to add a second commit to this PR that changes child to subprocess before the commit that then documents killed but I'd want to make sure there's consensus that this would be a good way to go.

@Trott Trott changed the title doc: add documentation for child.killed property doc: add documentation for killed property of ChildProcess instance Aug 1, 2017
@refack
Copy link
Contributor

refack commented Aug 1, 2017

PR that changes child to subprocess

Sounds like a good solution.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 2, 2017

+1 to changing to subprocess

@Trott
Copy link
Member Author

Trott commented Aug 2, 2017

One thing that changing to subprocess will do is break anchor links to our docs like https://nodejs.org/dist/latest-v8.x/docs/api/child_process.html#child_process_child_kill_signal. For that reason, I think it should be semver-major so that the change only lands when the URL is changing anyway because latest-v8.x is being bumped to latest-v9.x.

@jasnell
Copy link
Member

jasnell commented Aug 3, 2017

The anchor link breaks can be addressed by including <a id="..."></a> markup in the doc with the old generated ids.

@Trott
Copy link
Member Author

Trott commented Aug 3, 2017

The anchor link breaks can be addressed by including markup in the doc with the old generated ids.

I'm OK with that approach. I do have a slight preference for landing in Node.js 9.x and skipping the introduction of raw HTML into the markdown files though. This property has never been documented. It can wait another two months.

@Trott
Copy link
Member Author

Trott commented Aug 3, 2017

Oooh, maybe we can only add the HTML markup in backports so they never show up in master.

@Trott Trott force-pushed the doc-killed-property branch 2 times, most recently from 11c334e to 62aaa34 Compare August 3, 2017 18:19
@Trott
Copy link
Member Author

Trott commented Aug 3, 2017

OK, added the change to subprocess as a first commit. Doc subprocess.killed in the next commit. Instead of semver-major, I'm adding backport-requested for 6.x and 4.x. Once this goes through review and lands, I (or someone else if someone's motivated) can open backport commits that will add in the anchors so that existing URLs (from external sources to the 6.x and 4.x docs) don't break.

@Trott
Copy link
Member Author

Trott commented Aug 4, 2017

@cjihrig @lpinca Lots of changes since your approvals. You might want to take another look?

[`child.stderr`]: #child_process_child_stderr
[`child.stdin`]: #child_process_child_stdin
[`child.stdout`]: #child_process_child_stdout
[subprocess.connected`]: #child_process_child_connected
Copy link
Member

Choose a reason for hiding this comment

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

Missing opening backtick.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lpinca Oops, fixed, thanks!

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with missing backticks added.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % backtick fix

addaleax pushed a commit that referenced this pull request Aug 7, 2017
Backport-PR-URL: #14632
Backport-Reviewed-By: Refael Ackermann <[email protected]>
Backport-Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 7, 2017
Backport-PR-URL: #14632
Backport-Reviewed-By: Refael Ackermann <[email protected]>
Backport-Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 11, 2017
Backport-PR-URL: #14633
PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 11, 2017
Backport-PR-URL: #14633
PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 11, 2017
Backport-PR-URL: #14635
PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 11, 2017
Backport-PR-URL: #14635
PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 12, 2017
Backport-PR-URL: #14633
PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 12, 2017
Backport-PR-URL: #14633
PR-URL: #14578
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
@fastman
Copy link

fastman commented Aug 21, 2017

@Trott

PR that changes child to subprocess

Great! I've tried to suggest such change here #14444 (comment) but no one liked the idea then :/

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