GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-87452 Making returncode unreliability explicit by zitterbewegung · Pull Request #92181 · python/cpython

Conversation

@zitterbewegung

@zitterbewegung zitterbewegung commented May 2, 2022 •
edited by terryjreedy
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

We make explicit that the value stored in return code is best returned by poll instead of using the value in return code.

gh-87452: Making returncode unreliability explicit

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@zitterbewegung
Making unreliability of returncode explicit in documentation.
72565b8
@bedevere-bot

bedevere-bot commented May 2, 2022

Copy link
Copy Markdown

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels May 2, 2022
@zitterbewegung zitterbewegung changed the title Making returncode unreliability explicit gh-87452 Making returncode unreliability explicit May 2, 2022
@benjaminp

benjaminp commented May 2, 2022

Copy link
Copy Markdown
Contributor

Thank you for the PR.

I don't think "out-of-date" is the right term here. Either the entire subprocess.Subprocess instance is aware of the exit of the child process and knows the return code, or it doesn't. The first paragraph of the method documentation could perhaps be rearranged to more clearly state the order of events instead of appending another sentence.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
blurb-it Bot and others added 3 commits May 2, 2022 19:52
@blurb-it
📜🤖 Added by blurb_it.
8b7c0c3
@zitterbewegung
Rewording with benjaminp and abadger1999 suggestions.
c0dcc5b
@zitterbewegung
Merge remote-tracking branch 'refs/remotes/origin/pythongh-87452' into …
19390f4 
…pythongh-87452
@benjaminp

benjaminp commented May 2, 2022

Copy link
Copy Markdown
Contributor

I would propose:

The child return code. Initially ``None``, :attr:`returncode` is set by :meth:`poll` and :meth:`wait` after process termination is observed.
``
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zitterbewegung and others added 2 commits May 2, 2022 14:57
@zitterbewegung
Integrating rewording from benjaminp
d62de13
@blurb-it
📜🤖 Added by blurb_it.
903ae8d
benjaminp
benjaminp reviewed May 2, 2022
View reviewed changes
Comment thread Doc/library/subprocess.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/subprocess.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented May 3, 2022

Copy link
Copy Markdown
Contributor

Please remove the duplicate NEWS entry.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zitterbewegung added 3 commits May 3, 2022 12:25
@zitterbewegung
Merge branch 'python:main' into pythongh-87452
ecbe497
@zitterbewegung
Merge branch 'python:main' into pythongh-87452
c340c8f
@zitterbewegung
Merge branch 'python:main' into pythongh-87452
517dff7
slateny
slateny reviewed Oct 13, 2022
View reviewed changes
Comment thread Doc/library/subprocess.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
arhadthedev
arhadthedev reviewed Oct 13, 2022
View reviewed changes
Comment thread Doc/library/subprocess.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@zitterbewegung @arhadthedev
Update Doc/library/subprocess.rst
4adaaab 
Co-authored-by: Oleg Iarygin <[email protected]>
erlend-aasland
erlend-aasland requested changes Oct 14, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Again, please remove the duplicate NEWS entry. Also please address Benjamin's remarks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/subprocess.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Oct 14, 2022
@bedevere-bot

bedevere-bot commented Oct 14, 2022

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@slateny

slateny commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

@zitterbewegung Would you still be interested in giving the PR a quick update per the reviews above?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zitterbewegung

zitterbewegung commented Dec 6, 2022

Copy link
Copy Markdown
Contributor Author

Sorry I can do that.
slateny reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zitterbewegung
Updating docs for formatting issues.
ca4ea71 
Wrapping to 79 characters.
@bedevere-bot bedevere-bot mentioned this pull request Dec 6, 2022
[doc] Clarify that Popen.returncode does not get auto-set when the process terminates #87452
Closed
@netlify

netlify Bot commented Dec 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview ready!

Name Link
🔨 Latest commit 41a5d22
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/638f5503b72cc30008de175b
😎 Deploy Preview https://deploy-preview-92181--python-cpython-preview.netlify.app/whatsnew/changelog
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zitterbewegung
Updating for formatting issues
f7e1548 
Removing newline
zitterbewegung added 2 commits December 6, 2022 08:25
@zitterbewegung
Removing duplicate entry
6357654
@zitterbewegung
Removing duplicate news file.
708288c
zitterbewegung
zitterbewegung commented Dec 6, 2022
View reviewed changes

@zitterbewegung zitterbewegung left a comment

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

  • Fixed word wrap issue
  • Removed duplicate news entry.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@hugovk hugovk mentioned this pull request Dec 6, 2022
bpo-37860: re-add netlify.toml to set up deploy previews for docs #92852
Merged
@zitterbewegung
Removing trailing whitespace
41a5d22
zitterbewegung
zitterbewegung commented Dec 6, 2022
View reviewed changes

@zitterbewegung zitterbewegung left a comment

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed trailing whitespace

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@zitterbewegung

zitterbewegung commented Dec 6, 2022

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Dec 6, 2022
@bedevere-bot

bedevere-bot commented Dec 6, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

@erlend-aasland: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from erlend-aasland December 6, 2022 15:14
furkanonder
furkanonder approved these changes Jan 19, 2023
View reviewed changes
@zitterbewegung

zitterbewegung commented Apr 24, 2023

Copy link
Copy Markdown
Contributor Author

@erlend-aasland can this be reviewed?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead added 2 commits April 24, 2023 09:30
@gpshead
reword the Popen.returncode text
a6acc97
@gpshead
no need for a NEWS entry on a documentation edit
a4eef80
@gpshead gpshead self-assigned this Apr 24, 2023
@gpshead gpshead added skip news needs backport to 3.11 only security fixes labels Apr 24, 2023
gpshead added 2 commits April 24, 2023 09:36
@gpshead
Merge branch 'main' into pythongh-87452
e05f89f
@gpshead
- trailingspace
eb00171
@gpshead
gpshead merged commit 68f5836 into python:main Apr 24, 2023
@miss-islington

miss-islington commented Apr 24, 2023

Copy link
Copy Markdown
Contributor

Thanks @zitterbewegung for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting change review label Apr 24, 2023
@erlend-aasland

erlend-aasland commented Apr 25, 2023

Copy link
Copy Markdown
Contributor

Sorry for the delay; thanks Greg for merging. Thanks for the PR, Joshua.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@terryjreedy terryjreedy removed the needs backport to 3.11 only security fixes label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjaminp benjaminp benjaminp left review comments
@arhadthedev arhadthedev arhadthedev left review comments
@slateny slateny slateny left review comments
@furkanonder furkanonder furkanonder approved these changes
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@zitterbewegung @bedevere-bot @benjaminp @erlend-aasland @slateny @miss-islington @arhadthedev @furkanonder @gpshead @terryjreedy
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.