Skip to content

Conversation

@tlhunter
Copy link
Contributor

This PR documents the subprocess.ref() and subprocess.unref() methods. The latter method does appear in the documentation as a side node in the options.detached section while the prior does not appear anywhere.

Neither takes an argument nor has a return. I copied the documentation for unref from the existing detached docs. This is now redundant, perhaps the content should be removed from detached and a link be added to ref.

The introduction version number should be correct, see git log v0.7.10 to confirm.

Checklist

@nodejs-github-botnodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Aug 9, 2018
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be 0.7.10? And same for unref.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Whoops! Will fix.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe `for a given `subprocess` to exit`?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Change made

Copy link
Member

Choose a reason for hiding this comment

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

between the child and the parent?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Change made

Copy link
Contributor

@sagirksagirk left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

A few minor nits below.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ... before exiting itself. reads better, IMHO.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: forcing instead of allowing seems to be the right word here.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: given the context, usage of the word process to describe parent and child can be safely omitted.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

OK

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbytvsemozhetbyt added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Aug 10, 2018
@vsemozhetbyt
Copy link
Contributor

Landed in 7b1f3a4
Thank you!

vsemozhetbyt pushed a commit that referenced this pull request Aug 10, 2018
PR-URL: #22220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
targos pushed a commit that referenced this pull request Aug 11, 2018
PR-URL: #22220 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
@rvaggrvagg mentioned this pull request Aug 13, 2018
@tlhuntertlhunter deleted the child-process-doc branch August 13, 2018 17:11
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.child_processIssues and PRs related to the child_process subsystem.docIssues and PRs related to the documentations.fast-trackPRs that do not need to wait for 48 hours to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@tlhunter@vsemozhetbyt@bengl@jasnell@addaleax@TimothyGu@cjihrig@sagirk@lundibundi@BethGriggs@nodejs-github-bot