Skip to content

doc: clarify child_process.exec promise rejection on non-zero exit - #19541

Closed
TomCoded wants to merge 2 commits into
nodejs:masterfrom
TomCoded:il9494
Closed

doc: clarify child_process.exec promise rejection on non-zero exit#19541
TomCoded wants to merge 2 commits into
nodejs:masterfrom
TomCoded:il9494

Conversation

@TomCoded

@TomCoded TomCoded commented Mar 22, 2018

Copy link
Copy Markdown
Contributor

Promisify section of child_process.exec doc changed to make clear
that non-zero exit codes will result in promise rejection.

fixes: #19494

Checklist

@TomCoded
doc: clarify child_process.exec promise rejection on non-zero exit
f712774 
Promisify section of child_process.exec doc changed to make clear
that non-zero exit codes will result in promise rejection.

fixes: nodejs#19494
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to Node.js documentation. labels Mar 22, 2018
@vsemozhetbyt

vsemozhetbyt commented Mar 22, 2018

Copy link
Copy Markdown
Contributor

Thank you!

We have the similar warning for the child_process.execFile(). Should it be also updated?

@vsemozhetbyt vsemozhetbyt added promises Issues and PRs related to ECMAScript promises. fast-track PRs proposed for a shorter-than-standard waiting period before landing. labels Mar 22, 2018
benjamingr
benjamingr approved these changes Mar 22, 2018
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for doing this and welcome to Node :)

@TomCoded
doc: clarify child_process.execFile promise rejection on non-zero exit
f47c3bc 
Promisify section of child_process.execFile doc changed to make
clear that non-zero exit codes will result in promise rejection.

fixes: nodejs#19494
@TomCoded

TomCoded commented Mar 22, 2018

Copy link
Copy Markdown
Contributor Author

@vsemozhetbyt Good catch; added.

@benjamingr

benjamingr commented Mar 22, 2018

Copy link
Copy Markdown
Member

@TomCoded I'll elaborate on our process a little. This PR will remain open for 72 hours so collaborators and other people have time to review it. If after 72 hours it received no objections from a collaborator (there is a list in https://github.com/nodejs/node ) then we will land it :)

Thanks for your contribution - if you want to tackle more issues feel free to let me know and I'll help you find some more changes you're comfortable making :)

@vsemozhetbyt

vsemozhetbyt commented Mar 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@benjamingr It seems we can even fast-track this tiny doc change as per our new policy and not wait for 48/72 hours)

@vsemozhetbyt

vsemozhetbyt commented Mar 22, 2018

Copy link
Copy Markdown
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/290/

@vsemozhetbyt

vsemozhetbyt commented Mar 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Linting error is not related, see #19438 (comment)
However, we may need to wait for a green CI to be on the safe side)

@vsemozhetbyt

vsemozhetbyt commented Mar 22, 2018

Copy link
Copy Markdown
Contributor

CI-lite 2: https://ci.nodejs.org/job/node-test-pull-request-lite/294/

@vsemozhetbyt vsemozhetbyt added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 23, 2018
@benjamingr

benjamingr commented Mar 23, 2018

Copy link
Copy Markdown
Member

@vsemozhetbyt ah, very nice, I didn't realize we fast-track first contributions now :)

vsemozhetbyt pushed a commit that referenced this pull request Mar 23, 2018
@TomCoded @vsemozhetbyt
doc: clarify child_process promise rejections
43506f1 
Promisify sections of `child_process.exec()`
and `child_process.execFile()` changed to make clear
that non-zero exit codes will result in promise rejection.

PR-URL: #19541
Fixes: #19494
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@vsemozhetbyt

vsemozhetbyt commented Mar 23, 2018

Copy link
Copy Markdown
Contributor

Landed in 43506f1

Thank you, @TomCoded, and welcome to the Node.js contributors!

@tniessen tniessen removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 24, 2018
targos pushed a commit that referenced this pull request Mar 24, 2018
@TomCoded @targos
doc: clarify child_process promise rejections
8e3f59f 
Promisify sections of `child_process.exec()`
and `child_process.execFile()` changed to make clear
that non-zero exit codes will result in promise rejection.

PR-URL: #19541
Fixes: #19494
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@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

Reviewers

@jasnell jasnell jasnell approved these changes
@benjamingr benjamingr benjamingr approved these changes
+1 more reviewer
@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to Node.js documentation. fast-track PRs proposed for a shorter-than-standard waiting period before landing. promises Issues and PRs related to ECMAScript promises.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

promisify child_process.exec always rejects

6 participants

@TomCoded @vsemozhetbyt @benjamingr @jasnell @tniessen @nodejs-github-bot