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-41233: Add links to errnos referenced in exceptions docs #21380

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

@the-knights-who-say-ni the-knights-who-say-ni commented Jul 7, 2020

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
Member

@akuchling akuchling left a comment

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
Member

@akuchling akuchling Oct 19, 2020

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

@yyyyyyyan yyyyyyyan Oct 19, 2020

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
Member

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

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

@yyyyyyyan yyyyyyyan Oct 20, 2020

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
Member

@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

@yyyyyyyan yyyyyyyan Oct 20, 2020

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

@yyyyyyyan yyyyyyyan commented Jan 11, 2021

Hey @akuchling! Any updates on that?

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Looks good with some minor comments. Sorry this lingered for so long.

Doc/library/errno.rst Outdated Show resolved Hide resolved
Doc/library/exceptions.rst Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Apr 4, 2022

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
12 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 5, 2022

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

@bedevere-bot bedevere-bot commented Apr 5, 2022

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

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 5, 2022

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 issue Apr 5, 2022
…H-21380)

Co-authored-by: Andrew Kuchling <amk@amk.ca>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit a74892c)

Co-authored-by: yyyyyyyan <24644216+yyyyyyyan@users.noreply.github.com>
miss-islington added a commit that referenced this issue Apr 5, 2022
Co-authored-by: Andrew Kuchling <amk@amk.ca>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit a74892c)

Co-authored-by: yyyyyyyan <24644216+yyyyyyyan@users.noreply.github.com>
miss-islington added a commit that referenced this issue Apr 5, 2022
Co-authored-by: Andrew Kuchling <amk@amk.ca>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit a74892c)

Co-authored-by: yyyyyyyan <24644216+yyyyyyyan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA signed docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants