Skip to content

bpo-41233: Add links to errnos referenced in exceptions docs #21380

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

Merged
merged 7 commits into from
Apr 5, 2022

Conversation

yyyyyyyan
Copy link
Contributor

@yyyyyyyan yyyyyyyan commented Jul 7, 2020

Links the errnos referenced in Doc/library/exceptions.rst to their respective section in Doc/library/errno.rst, and creates a "see also" box in these errnos sections in Doc/library/errno.rst linking them to their mapped exceptions in Doc/library/exceptions.rst

https://bugs.python.org/issue41233

@the-knights-who-say-ni
Copy link

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 Missing

Our records indicate the following people have not signed the CLA:

@yyyyyyyan

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
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Copy link
Contributor

@akuchling akuchling 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 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.

@@ -29,16 +29,25 @@ defined by the module. The specific list of defined symbols is available as

Operation not permitted

.. seealso::
Copy link
Contributor

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.

Copy link
Contributor Author

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`.

Copy link
Contributor

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?

Copy link
Contributor Author

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!

Copy link
Contributor

@akuchling akuchling Oct 20, 2020

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.

Copy link
Contributor Author

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 :-)

@yyyyyyyan
Copy link
Contributor Author

Hey @akuchling! Any updates on that?

Copy link
Member

@JelleZijlstra JelleZijlstra 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 with some minor comments. Sorry this lingered for so long.

@JelleZijlstra JelleZijlstra self-assigned this Apr 4, 2022
@JelleZijlstra JelleZijlstra added the needs backport to 3.10 only security fixes label Apr 4, 2022
@JelleZijlstra
Copy link
Member

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.

@JelleZijlstra JelleZijlstra merged commit a74892c into python:main Apr 5, 2022
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-32316 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Apr 5, 2022
@bedevere-bot
Copy link

GH-32317 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 5, 2022
…H-21380)

Co-authored-by: Andrew Kuchling <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
(cherry picked from commit a74892c)

Co-authored-by: yyyyyyyan <[email protected]>
miss-islington added a commit that referenced this pull request Apr 5, 2022
Co-authored-by: Andrew Kuchling <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
(cherry picked from commit a74892c)

Co-authored-by: yyyyyyyan <[email protected]>
miss-islington added a commit that referenced this pull request Apr 5, 2022
Co-authored-by: Andrew Kuchling <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
(cherry picked from commit a74892c)

Co-authored-by: yyyyyyyan <[email protected]>
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
…H-21380)

Co-authored-by: Andrew Kuchling <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
(cherry picked from commit a74892c)

Co-authored-by: yyyyyyyan <[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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants