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

gh-101100: Fix broken xrefs in fcntl module doc #115691

Merged
merged 7 commits into from
Feb 25, 2024

Conversation

smontanaro
Copy link
Contributor

@smontanaro smontanaro commented Feb 19, 2024

A few changes:

  1. Links for fcntl, ioctl and flock system calls are suppressed. Sphinx only complained about some, but even if it catalogued these somehow, it would likely have been incorrect.
  2. Suppress all :const:LOCK_* refs. They appear nowhere else in the Python docs. Users should read the relevant Unix manpages.
  3. Convert EACCES an EAGAIN constant references to data references in the errno module.

I'm inclined to move the rather large set of :versionchanged: directives down near the bottom of the file, though for now I left them alone. Almost all of them just identify additions to the set of command constants, and are both platform- and Linux kernel-dependent. As such, they are probably going to be used infrequently. They aren't key to understanding how to use the module.


📚 Documentation preview 📚: https://cpython-previews--115691.org.readthedocs.build/

Doc/library/fcntl.rst Outdated Show resolved Hide resolved
@smontanaro
Copy link
Contributor Author

Thanks @gpshead. Can you merge? I'll take care of any conflicts backporting to 3.11 or 3.12.

@CAM-Gerlach CAM-Gerlach self-requested a review February 20, 2024 19:54
@CAM-Gerlach CAM-Gerlach added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Feb 20, 2024
@CAM-Gerlach CAM-Gerlach changed the title gh-101100: clean up fcntl module doc gh-101100: Fix broken xrefs in fcntl module doc Feb 20, 2024
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.

Standard reminder: You can apply all desired suggestions with one click by going to the Files changed tab, clicking Add to batch on each suggestion you want, and clicking Commit once you're done. [Note some comments have both an applyable suggestion and one should be copy/pasted elsewhere]

Thanks @smontanaro ! Besides the individual suggestions, as we discussed on the Python Discourse, you mentioned you're going to:

  • Add ('c:func', 'ioctl) to nitpick_ignore in conf.py
  • Remove manual silencing of (already ignored) fcntl and to be silenced ioctl

(I'm also to take care of that for you in a commit to your branch, if you're busy)

Add additionally, I recommend:

  • Add minimal documentation for LOCK_* constants (copy/pastable suggestions included below)—or at the very least, I don't think we should hide valid warnings due to missing documentation for public module constants used by a public function :)

Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Feb 21, 2024

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.

@smontanaro
Copy link
Contributor Author

I pushed two small changes to fcntl.rst and conf.py. @CAM-Gerlach take a look.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @smontanaro.

Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
Doc/library/fcntl.rst Outdated Show resolved Hide resolved
CAM-Gerlach
CAM-Gerlach previously approved these changes Feb 23, 2024
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.

Standard reminder: You can directly apply all the suggestions you want in one go by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit

Thanks, looking great now! I'm definitely going to steal borrow your approach here with embedding data within the function def for function-specific constants like this that aren't formally documented on their own.

Just a couple tweaks, all as one-click applyable suggestions, to fix a typo, formatting per the style guide, minimize diffs and add one tiny clarification; otherwise LGTM. Thanks!

@CAM-Gerlach CAM-Gerlach dismissed their stale review February 23, 2024 02:03

GitHub screwed up again and didn't submit my final fixup suggestions with my review >:(

@willingc
Copy link
Contributor

@CAM-Gerlach Do you want to accept all of your suggestions for @smontanaro and go ahead and merge this PR since it seems ready to go? Oh, and nice job giving clear suggestions and concise steps.

@smontanaro
Copy link
Contributor Author

@CAM-Gerlach I find the display of suggestions challenging, often because GitHub doesn't display enough context. Also, to make sure readers know what you're talking about, I suggest minimizing the use of pronouns to identify items you want to see changed. I'll go over to the Files tab now and see what's being suggested.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Looks like all of the suggestions were added.

Thanks @smontanaro. Merging.

@willingc willingc merged commit 84a275c into python:main Feb 25, 2024
22 checks passed
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

Sorry, @smontanaro and @willingc, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 84a275c4a2c8a22d198c6f227d538e6b27bbb029 3.12

@miss-islington-app
Copy link

Sorry, @smontanaro and @willingc, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 84a275c4a2c8a22d198c6f227d538e6b27bbb029 3.11

@willingc
Copy link
Contributor

I'm having issues with cherry_picker saying "You're not inside a cpython repo right now! 🙅" 😢 @Mariatta do you know what causes that error?

@hugovk
Copy link
Member

hugovk commented Feb 25, 2024

It's probably python/cherry-picker#99.

Try: git config --local --remove-section cherry-picker

@bedevere-app
Copy link

bedevere-app bot commented Feb 26, 2024

GH-115924 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Feb 26, 2024
willingc pushed a commit to willingc/cpython that referenced this pull request Feb 26, 2024
…H-115691)

* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review

Co-authored-by: C.A.M. Gerlach <[email protected]>

---------

(cherry picked from commit 84a275c)

Co-authored-by: Skip Montanaro <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Feb 26, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 26, 2024
willingc added a commit that referenced this pull request Feb 26, 2024
…115924)

* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review



---------

(cherry picked from commit 84a275c)

Co-authored-by: Skip Montanaro <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
willingc added a commit that referenced this pull request Feb 26, 2024
…115925)

* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review



---------

(cherry picked from commit 84a275c)

Co-authored-by: Skip Montanaro <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Feb 26, 2024

Thanks @smontanaro. Merging.

Thanks for taking care of that, @willingc — I'd gotten busy with a proposal due Tuesday and following up here slipped through the cracks, sorry.

@CAM-Gerlach Do you want to accept all of your suggestions for @smontanaro and go ahead and merge this PR since it seems ready to go?

I'd left the suggestions along with an approval, as I typically do when the changes are minor and everything else is LGTM and ready the author (or any core dev, if they aren't one themselves) to merge after applying them. Personally, I (and my colleagues on other projects, at least) wouldn't consider it appropriate to modify an author's branch themselves without their permission under normal circumstances unless they become unresponsive, and outside of that its not something I see regularly done here either.

Unfortunately, GitHub screwed things up and instead of submitting an approving review with a few minor suggestions, it instead submitted the suggestions first in two separate batches, and then just the approval, so to avoid any confusion I dismissed the latter.

Oh, and nice job giving clear suggestions and concise steps.

Thanks, just doing what I've always done on my countless prior reviews like this one. I've always used explicit suggestions whenever practical vs. requiring the author to figure out what I'm asking, implement it manually themselves and hope they get it right, as it saves both me and them a ton of time, effort and frustration. I just copy/paste in the boilerplate line reminding authors how to batch-apply them if they wish (as even some core devs need a refresher sometimes), and I like to briefly summarize my proposed changes and the reasons for them so authors can understand and evaluate them (though occasionally I've gotten feedback that they may be too much for some people).

@CAM-Gerlach I find the display of suggestions challenging, often because GitHub doesn't display enough context. Also, to make sure readers know what you're talking about, I suggest minimizing the use of pronouns to identify items you want to see changed. I'll go over to the Files tab now and see what's being suggested.

As an author I normally always review suggestions in the Files tab anyway which gives you unlimited context, so I'm not sure how much that really matters if you're doing that? And I'm sorry if there was confusion; I wasn't requesting anything else be changed, just giving a brief bullet point summary of the change(s) in each suggestion and the reason for them. Or are you referring to an earlier review—if so, could you point me to that so I can see what you mean? Thanks.

@willingc
Copy link
Contributor

willingc commented Feb 26, 2024

For future zen, let's lean a bit more into "explicit instead of implicit". GitHub's UI can make a mess of reviews and doesn't always convey context from the reviewer to the PR author.

For seasoned core devs/contributors:

  • I signal "nice to have" suggestions by approving the PR and stating so.
  • I signal "important to have" (or process has recently changed) by using comment review status
  • I signal "will break or very confusing" with the changes requested status

I generally lean toward giving the seasoned PR contributor the most straightforward path to merge (our time is precious). We can always follow up with a "touch-up" PR.

Thanks @smontanaro and @CAM-Gerlach. It's great to have these improvements in. 💐

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review

Co-authored-by: C.A.M. Gerlach <[email protected]>

---------

Co-authored-by: C.A.M. Gerlach <[email protected]>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review

Co-authored-by: C.A.M. Gerlach <[email protected]>

---------

Co-authored-by: C.A.M. Gerlach <[email protected]>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
* clean up fcntl module doc

* simplify

* a few changes, based on suggestion by CAM-Gerlach

* nitpick ignore for a couple other C functions mentioned in the fcntl module doc

* more changes, especially related to LOCK_* constants

* :data: back to :const:

* Apply suggestions from code review

Co-authored-by: C.A.M. Gerlach <[email protected]>

---------

Co-authored-by: C.A.M. Gerlach <[email protected]>
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 skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants