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-101578: Amend PyErr_{Set,Get}RaisedException docs #101962

Merged
merged 6 commits into from Feb 19, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Feb 16, 2023

@erlend-aasland
Copy link
Contributor Author

  • I removed the duplicate example; it should be sufficient to provide a single example.
  • Use note:: to put emphasis on non-standard reference count semantics in the set function.
  • Mark the getter as returning a borrowed reference.
  • Apply Diátaxis: be succinct and to the point (and hopefully also clearer)

Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
@CAM-Gerlach CAM-Gerlach added the docs Documentation in the Doc dir label Feb 17, 2023
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.

Thanks @erlend-aasland ; this is a substantial improvement to the previous docs.

Perhaps its worth fixing the other couple remaining similar docs defects also introduced in PR #101607 , namely in the descriptions of the newly documented functions PyException_GetArgs and PyException_SetArgs (that could use a similar (if much simpler) treatment as here), and the deprecation notices added to PyErr_Fetch, PyErr_Restore and PyErr_NormalizeException not being added to, and particularly duplicating, the .. deprecated directive.

Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

Thanks @erlend-aasland ; this is a substantial improvement to the previous docs.

Thanks, and thanks for the review!

Perhaps its worth fixing the other couple remaining similar docs defects also introduced in PR #101607 , namely in the descriptions of the newly documented functions PyException_GetArgs and PyException_SetArgs (that could use a similar (if much simpler) treatment as here), and the deprecation notices added to PyErr_Fetch, PyErr_Restore and PyErr_NormalizeException not being added to, and particularly duplicating, the .. deprecated directive.

Definitely; perhaps as a follow-up PR?

@CAM-Gerlach
Copy link
Member

Definitely; perhaps as a follow-up PR?

Yeah certainly could be; up to you, of course

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.

A couple followup suggestions

Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
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.

LGTM from me; thanks @erlend-aasland !

@erlend-aasland
Copy link
Contributor Author

Thanks for the review, CAM; highly appreciated. I'll wait for @iritkatriel's or @markshannon's approval before landing.

Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Doc/c-api/exceptions.rst Outdated Show resolved Hide resolved
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@erlend-aasland
Copy link
Contributor Author

Thanks for the reviews, Irit and CAM; highly appreciated. I'll create a follow-up PR Any Time Soon™.

@erlend-aasland erlend-aasland added 🤖 automerge PR will be merged once it's been approved and all CI passed and removed 🤖 automerge PR will be merged once it's been approved and all CI passed labels Feb 19, 2023
@erlend-aasland erlend-aasland merged commit 60bbed7 into python:main Feb 19, 2023
15 checks passed
@erlend-aasland erlend-aasland deleted the fixup-exc-docs branch February 19, 2023 20:22
erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Feb 19, 2023
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.

None yet

4 participants