Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-24888: Clarify subprocess.check_call propagates exceptions if unable to start process#28018

Merged
ambv merged 7 commits into
python:mainpython/cpython:mainfrom
DonnaDia:fix-issue-24888DonnaDia/cpython:fix-issue-24888Copy head branch name to clipboard
Sep 7, 2021
Merged

bpo-24888: Clarify subprocess.check_call propagates exceptions if unable to start process#28018
ambv merged 7 commits into
python:mainpython/cpython:mainfrom
DonnaDia:fix-issue-24888DonnaDia/cpython:fix-issue-24888Copy head branch name to clipboard

Conversation

@DonnaDia

@DonnaDia DonnaDia commented Aug 28, 2021

Copy link
Copy Markdown
Contributor

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir labels Aug 28, 2021
@DonnaDia DonnaDia changed the title bpo-24388: FileNotFoundException raised by subprocess.call bpo-24888: FileNotFoundException raised by subprocess.call Aug 28, 2021
Comment thread Doc/library/subprocess.rst Outdated
Comment thread Doc/library/subprocess.rst Outdated

@Fidget-Spinner Fidget-Spinner 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.

Thanks for implementing the changes.

FYI, if you haven't read this already, I highly recommend learning how to build the documentation locally https://devguide.python.org/documenting/#building-the-documentation to save yourself some time. That entire page also teaches basic reStructuredText, so I recommend reading it in full if you have the time.

Comment thread Doc/library/subprocess.rst Outdated
Comment thread Doc/library/subprocess.rst Outdated
:exc:`CalledProcessError` object will have the return code in the
:attr:`~CalledProcessError.returncode` attribute.

If :func:`check_call` was unable to start the process it will return the error

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.

Will it return an error? or propagate whatever exception was raised?

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.

Actually I just notice there is duplication between this paragraph and the previous one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Shall we leave the rephrased version if it is more obvious?

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.

I think the original is more precise w.r.t the wrapped returncode value.

I would leave the original paragraph and just append to it a sentence or half-sentence about what happens if the child process could not even start. This order seems more natural to me: the common case is your job succeeded, then that your job failed, then the least common case is that your job could not even launch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added the changes

@DonnaDia DonnaDia requested a review from iritkatriel September 5, 2021 17:57
@ambv ambv changed the title bpo-24888: FileNotFoundException raised by subprocess.call bpo-24888: Clarify subprocess.check_call propagates exceptions if unable to start process Sep 7, 2021
@ambv ambv added the skip news label Sep 7, 2021
@ambv ambv merged commit 19a304b into python:main Sep 7, 2021
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @DonnaDia for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @DonnaDia for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot

Copy link
Copy Markdown

GH-28223 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 7, 2021
…ble to start process (pythonGH-28018)

(cherry picked from commit 19a304b)

Co-authored-by: DonnaDia <37962843+DonnaDia@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 7, 2021
…ble to start process (pythonGH-28018)

(cherry picked from commit 19a304b)

Co-authored-by: DonnaDia <37962843+DonnaDia@users.noreply.github.com>
@bedevere-bot

Copy link
Copy Markdown

GH-28224 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Sep 7, 2021
ambv pushed a commit that referenced this pull request Sep 7, 2021
…ble to start process (GH-28018) (GH-28223)

(cherry picked from commit 19a304b)

Co-authored-by: DonnaDia <37962843+DonnaDia@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Sep 7, 2021
…ble to start process (GH-28018)

(cherry picked from commit 19a304b)

Co-authored-by: DonnaDia <37962843+DonnaDia@users.noreply.github.com>
@DonnaDia DonnaDia deleted the fix-issue-24888 branch September 7, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review docs Documentation in the Doc dir skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.