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.

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

bpo-41825: restructure docs for the os.wait*() family #22356

Merged
merged 8 commits into from
Nov 28, 2022
Merged

bpo-41825: restructure docs for the os.wait*() family #22356

merged 8 commits into from
Nov 28, 2022

Conversation

birkenfeld
Copy link
Member

@birkenfeld birkenfeld commented Sep 22, 2020

Mostly fixes up the specific docs of waitid, so that basic usage and all flags are explained without having to consult the manpage.

While doing that, I noticed a few issues with the other wait* functions. I tried to reorder them and W* constants to be sequential, and clearly mark which flag can be used for which function.

https://bugs.python.org/issue41825

Fixes #85991

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Thanks for the significant improvements to the os.wait*() docs, @birkenfeld!

I verified the new waitid() content against the following man page, as well as locally verifying the cases where ChildProcessError is raised. Other than a few specific parts where I have suggestions, it mostly looks good to me (it would be a very clear improvement as-is, so certainly feel free to include some or none of my suggestions if you disagree with any).

Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

This has merge conflicts now.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@birkenfeld
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@iritkatriel: please review the changes made to this pull request.

@iritkatriel
Copy link
Member

The doc build has errors.

@iritkatriel iritkatriel dismissed their stale review November 25, 2022 20:55

merge conflicts were resolved, but I did not re-review so not ready to approve.

Doc/library/os.rst Outdated Show resolved Hide resolved
Co-authored-by: Kyle Stanley <aeros167@gmail.com>
@CAM-Gerlach
Copy link
Member

Just FYI, the force-push invalidated my mostly-complete review, so it will be some additional time while I recover and remake it.

@iritkatriel
Copy link
Member

Just FYI, the force-push invalidated my mostly-complete review, so it will be some additional time while I recover and remake it.

Yeah, it's better to just push with additional commits (not force push). This way the reviewer can see what changed since the last review. We squash the commits when we merge so it doesn't matter.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Overall, this seems like a quite helpful change. I did have a some questions and comments to clarify points that I as a non-expert reader found ambiguous, confusing, unclear or potentially contradictory, and a number of suggestions with textual and reST syntax tweaks to fix typos/grammar issues, fix/improve the reST syntax, clarify the text and be consistent with current docs conventions. Thanks!

FYI, you might already be aware, but you can easily apply a bunch of suggestions at once by going to the Files changed tab, clicking Add to batch on each suggestion you want and then clicking Commit. You can also reply with your own modified suggestions, add those and commit them instead at the same time.

Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
birkenfeld and others added 2 commits November 26, 2022 06:53
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
birkenfeld and others added 3 commits November 26, 2022 06:55
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@birkenfeld
Copy link
Member Author

Thanks for the review! All your suggestions/comments should be addressed.

@CAM-Gerlach CAM-Gerlach added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Nov 26, 2022
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Rechecked the source and the rendered output, and it LGTM now from my end, aside from a couple tiny nits. Thanks again!

Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@birkenfeld
Copy link
Member Author

Sorry for missing these, added.

@CAM-Gerlach
Copy link
Member

Sorry for missing these, added.

No need to be sorry—it was me who missed one, if not both of them :)

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM from my end; thanks @birkenfeld !

@iritkatriel
Copy link
Member

Thank you @birkenfeld, @aeros and @CAM-Gerlach.

@iritkatriel iritkatriel merged commit 492dc02 into python:main Nov 28, 2022
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 28, 2022
(cherry picked from commit 492dc02)

Co-authored-by: Georg Brandl <georg@python.org>
@bedevere-bot
Copy link

GH-99840 is a backport of this pull request to the 3.11 branch.

@miss-islington
Copy link
Contributor

Sorry, @birkenfeld and @iritkatriel, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 492dc02b01828f346dd62412fefc654e781de923 3.10

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Nov 28, 2022
miss-islington added a commit that referenced this pull request Nov 28, 2022
(cherry picked from commit 492dc02)

Co-authored-by: Georg Brandl <georg@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir needs backport to 3.10 only security fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

os.waitid() documentation needs TLC
8 participants