-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
Conversation
Thanks @gpshead. Can you merge? I'll take care of any conflicts backporting to 3.11 or 3.12. |
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.
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
) tonitpick_ignore
inconf.py
- Remove manual silencing of (already ignored)
fcntl
and to be silencedioctl
(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 :)
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 pushed two small changes to |
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.
Looks good. Thanks @smontanaro.
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.
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!
GitHub screwed up again and didn't submit my final fixup suggestions with my review >:(
@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. |
@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. |
Co-authored-by: C.A.M. Gerlach <[email protected]>
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.
Looks like all of the suggestions were added.
Thanks @smontanaro. Merging.
Thanks @smontanaro for the PR, and @willingc for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
Sorry, @smontanaro and @willingc, I could not cleanly backport this to
|
Sorry, @smontanaro and @willingc, I could not cleanly backport this to
|
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? |
It's probably python/cherry-picker#99. Try: |
GH-115924 is a backport of this pull request to the 3.12 branch. |
…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]>
GH-115925 is a backport of this pull request to the 3.11 branch. |
…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]>
…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]>
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.
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.
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).
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. |
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 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. 💐 |
* 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]>
* 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]>
* 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]>
A few changes:
fcntl
,ioctl
andflock
system calls are suppressed. Sphinx only complained about some, but even if it catalogued these somehow, it would likely have been incorrect.:const:LOCK_*
refs. They appear nowhere else in the Python docs. Users should read the relevant Unix manpages.EACCES
anEAGAIN
constant references to data references in theerrno
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/