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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

TomCoded
Copy link
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 the documentations. labels Mar 22, 2018
@vsemozhetbyt
Copy link
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 that do not need to wait for 48 hours to land. labels Mar 22, 2018
benjamingr
benjamingr approved these changes Mar 22, 2018
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

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
Copy link
Contributor Author

@vsemozhetbyt Good catch; added.

@benjamingr
Copy link
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
Copy link
Contributor

vsemozhetbyt commented Mar 22, 2018 •
edited
Loading

@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
Copy link
Contributor

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Mar 22, 2018 •
edited
Loading

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
Copy link
Contributor

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

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 23, 2018
@benjamingr
Copy link
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
Copy link
Contributor

Landed in 43506f1

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

@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. 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