-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-83785: Document missing PyDateTime* APIs.
#141543
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
base: main
Are you sure you want to change the base?
gh-83785: Document missing PyDateTime* APIs.
#141543
Conversation
Doc/c-api/datetime.rst
Outdated
|
|
||
| .. c:macro:: PyDateTime_IMPORT() | ||
| Import the datetime C API. The macro does not need a semi-colon to be called. |
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 macro does not need a semi-colon to be called.
This is wrong.
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.
Yes, I should've fact-checked this one before copy-pasting it. Good catch!
| .. code-block:: | ||
| PyDateTime_IMPORT(); // semi-colon is optional but recommended | ||
| if (PyErr_Occurred()) { /* cleanup */ } |
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 (PyErr_Occurred()) { /* cleanup */ } | |
| if (PyDateTimeAPI == NULL) { /* cleanup */ } |
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? We should probably avoid encouraging the use of global variables here.
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.
This is the current practice in CPython.
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.
Is there a situation where PyDateTime_API is null that PyErr_Occurred() does not cover, or vice versa (i.e. partially initialized PyDateTimeAPI?)
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.
Per the PyCapsule_Import docs and a glance over it's source:
On failure, set an exception and return NULL.
So the checks are equivalent.
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
|
This sounded familiar so I thought there was some reason that we didn't do it, but apparently I actually wanted to go even further than this and didn't get around to it / didn't drum up enough interest: #83785 I am tempted to say we should not say the thing about the struct contents being private and subject to change because there are situations where they are legitimately the only way to accomplish things and I am not crazy about the fact that our public interface is C macros anyway (given that those macros don't work outside of C++, I'd rather they be symbols that can be linked against in an FFI). Given the subinterpreter stuff, though, it might be a bit of an own goal to make the struct public right away before making a plan for how to move forward (in case the implementation details of the struct end up being inconvenient in that effort), so I'm +1 on the current scope of this PR. |
PyDateTime* APIs.PyDateTime* APIs.
Oh I forgot about that one! I've moved this PR under it, since it is more specific. |
I was mostly inspired by Bénédikt's work on the
cursesC API (#141254) for this.In the future, I think we should completely deprecate
PyDateTimeAPIandPyDateTime_IMPORTbecause of subinterpreter incompatibility, as well as documenting all the fields onPyDateTime_CAPIto make this usable under subinterpreters, but that's a later project.📚 Documentation preview 📚: https://cpython-previews--141543.org.readthedocs.build/en/141543/c-api/datetime.html