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-41233: Add links to errnos referenced in exceptions docs #21380
Conversation
…xceptions referenced in errnos.rst
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Reword news item a bit.
Looks good to me. I ran the formatting, clicked between errno and exceptions a bunch, and then manually inspected the diffs to be sure that an incorrect error code hadn't crept in. This patch was originally against 3.8, so I glanced at the NEWS to check that we haven't introduced any new errno/exception mappings.
Doc/library/errno.rst
Outdated
@@ -29,16 +29,25 @@ defined by the module. The specific list of defined symbols is available as | |||
|
|||
Operation not permitted | |||
|
|||
.. seealso:: |
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.
The seealso:: notation adds a yellow box as a highlight, which is maybe a little too prominent -- I wondered if the links should just be part of the tiny paragraph. ex. ("Operation not permitted. This error is mapped to ..."). But 'seealso' is certainly the right logical concept to use, and EINTR/InterruptedError already set the pattern, so I think this approach is correct.
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.
I followed the pattern set by EINTR/InterruptedError, but I agree it may be a bit much, visually. Maybe it'd be better if we did as in the InterruptedError description in exceptions.rst, and just add the observation to the description instead of leaving it as a 'See Also' extra.
So we have the following in exceptions.rst:
.. exception:: InterruptedError
Raised when a system call is interrupted by an incoming signal.
Corresponds to :c:data:`errno` :py:data:`~errno.EINTR`.
And we could do the same in errno.rst:
.. data:: EINTR
Interrupted system call.
This error is mapped to the exception :exc:`InterruptedError`.
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.
I went ahead and tried it in just paragraph form, and I think it looks fine. Because I had to do the necessary editing for the experiment anyway, I've gone ahead and pushed the commit to your branch -- no point making you re-do the same work! What do you think?
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.
I think it's great, really, thanks a lot for this!
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.
@yyyyyyyan: thanks! because you're a first-time contributor, you can also add yourself to the Misc/ACKS file if you like. (It's sorted alphabetically by last name.) Once you've done that (or said you prefer not to be added) I'll finally merge this.
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.
Oh, that's great, I sure want it! Just committed the addition to Misc/ACKS :-)
Hey @akuchling! Any updates on that? |
Looks good with some minor comments. Sorry this lingered for so long.
Misc/NEWS.d/next/Documentation/2020-07-07-22-54-51.bpo-41233.lyUJ8L.rst
Outdated
Show resolved
Hide resolved
Sorry this lingered, it seems like @akuchling hasn't been active. This is a very useful change and I'm planning to merge it once CI passes. |
Thanks @yyyyyyyan for the PR, and @JelleZijlstra for merging it |
GH-32316 is a backport of this pull request to the 3.10 branch. |
GH-32317 is a backport of this pull request to the 3.9 branch. |
Links the errnos referenced in
Doc/library/exceptions.rst
to their respective section inDoc/library/errno.rst
, and creates a "see also" box in these errnos sections inDoc/library/errno.rst
linking them to their mapped exceptions inDoc/library/exceptions.rst
https://bugs.python.org/issue41233