-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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: Clean up Doc/c-api/exceptions.rst and Doc/c-api/sys.rst #114825
Conversation
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.
If you want to specify their declaration, you can simply use typedef-like syntax.
I would move the Py_AuditHookFunction
definition directly into PySys_AddAuditHook
definition where it is only used. You need to add .. c:namespace:: NULL
before it to avoid adding a prefix.
The PyOS_sighandler_t
definition can be moved closer to PyOS_getsig()
or PyOS_setsig()
definitions. Duplicated description of PyOS_sighandler_t
can be removed.
Thanks @serhiy-storchaka, I believe I folded in your suggestions in a reasonable way. |
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.
It deviated significantly from initial "clean up Doc/c-api/exceptions.rst", isn't? 😉 Maybe split it on several PRs? The auditing part is not trivial.
LGTM after you do something with duplication for Py_AuditHookFunction
. For example:
.. c:namespace:: NULL
.. c:type:: int (*Py_AuditHookFunction) (const char *event, PyObject
*args, void *userData)
The type of the hook function.
*event* is (the event argument passed to PySys_Audit() or PySys_AuditTuple()),
*args* is (...), *userData* is (the argument passed to PySys_AddAuditHook()).
The hook function is always called ...
See :pep:`578` for ...
Feature creep is no problem. I'm retired, so have plenty of time to mess around. ;-) Since it seems like I might have to write an actual sentence to flesh out this paragraph, I'm going to spend a bit of time reading PEP 578, but I'll have something later today. |
Doc/c-api/sys.rst
Outdated
.. c:type:: int (*Py_AuditHookFunction) (const char *event, PyObject *args, void *userData) | ||
|
||
The type of the hook function. | ||
*event* is (the event argument passed to PySys_Audit() or PySys_AuditTuple()), |
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.
Wait, I used parentheses to denote that it is a preliminary text, and that you should rewrite it in good English. Perhaps it is even better to describe parameters not in terms of PySys_Audit() or PySys_AuditTuple(), but in more general terms. Look at sys.audit()
. Perhaps "event is a C string identifying the event" is enough.
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.
Ah, I recognized "(...)" as a placeholder, not the more elaborate parenthesized phrasing. Will get back to it.
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.
Perhaps "event is a C string identifying the event" is enough.
I thought about that, but am not familiar enough with the C API documentation to know that descriptions of function prototypes don't simply rely on the actual prototype.
Doc/c-api/sys.rst
Outdated
.. c:namespace:: NULL | ||
.. c:type:: int (*Py_AuditHookFunction) (const char *event, PyObject *args, void *userData) |
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.
If it is not nested, c:namespace
is not needed. I would prefer to make it nested, but it is only my personal preference.
Doc/c-api/sys.rst
Outdated
If the interpreter is initialized, this function raises an auditing event | ||
``sys.addaudithook`` with no arguments. If any existing hooks raise an | ||
exception derived from :class:`Exception`, the new hook will not be | ||
added and the exception is cleared. As a result, callers cannot assume | ||
that their hook has been added unless they control all existing hooks. |
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.
Why is it dedented? It belongs to the audit-event
directive.
Sorry, I clearly don't understand the functional implications of the indentation. See if this latest version is more 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.
LGTM.
Thanks @smontanaro for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
…rst (pythonGH-114825) (cherry picked from commit e1552fd) Co-authored-by: Skip Montanaro <skip.montanaro@gmail.com>
Sorry, @smontanaro and @serhiy-storchaka, I could not cleanly backport this to
|
GH-115308 is a backport of this pull request to the 3.12 branch. |
@serhiy-storchaka I'll take care of the 3.11 conflict (I need the practice). |
…rst (pythonGH-114825) (cherry picked from commit e1552fd)
GH-115311 is a backport of this pull request to the 3.11 branch. |
This PR cleans up several bits in
Doc/c-api/exceptions.rst
:warnings.WarningMessage
. It's not actually documented and isn't exposed bywarnings.__all__
. It's just a basic exception carrying around a message.__module__
attribute. The good stuff is in the containing paragraph.USE_STACKCHECK
. The good stuff is in the documentation ofPyOS_CheckStack
, which is linked from the containing paragraph.PyExc_OSError
.Since I was hunting down
USE_STACKCHECK
, I went ahead and suppressed the reference inDoc/c-api/sys.rst
, in the documentation forPyOS_CheckStack
.I updated
Doc/tools/.nitignore
to removeDoc/c-api/exceptions.rst
, but left an entry forDoc/c-api/sys.rst
, as I didn't make an attempt to address any other problems it might have.📚 Documentation preview 📚: https://cpython-previews--114825.org.readthedocs.build/