Added some more info on PyModule_GetState doc. #138823
Conversation
|
All commit authors signed the Contributor License Agreement. |
bedevere-app
bot
added
awaiting review
docs
Doc/c-api/module.rst
Outdated
| allocated at module creation time, or ``NULL``. See | ||
| :c:member:`PyModuleDef.m_size`. | ||
| The module’s state can be stored in per‑module memory, which is obtained by calling this function. | ||
| This makes modules safe to use with sub‑interpreters. |
There was a problem hiding this comment.
| This makes modules safe to use with sub‑interpreters. |
This is too specific.
There was a problem hiding this comment.
But isn't it the most useful feature of this function? Otherwise it doesn't makes sense to store per-module state.
There was a problem hiding this comment.
Maybe instead link to the PEP?
There was a problem hiding this comment.
Sorry, but I disagree. PEPs shouldn't be used as documentation, because they're solely historical documents and don't receive updates when we change things. I think we should keep the "This makes modules safe to use with sub‑interpreters" sentence, because yes -- that's what this function is for.
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
I suggest leave things as is. There's no issue.
Maybe just add PEP link, indeed.
s3rius
force-pushed
the
feat/docs-enhance
branch
from
bf028b2 to
c65633d
Compare
|
@skirpichev, @sobolevn. Intead of all the changes added link to the respective PEP. Can you please take a look once again? |
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
I'm still not sure if that's helpful. PEP 489 introduces a lot of API, why mention it just there? And also PyModule_GetState() was added by a different PEP, IIRIC.
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Please don't solely link to the PEP. I think the original sentence ("This makes modules safe to use with sub‑interpreters") was fine.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: Stan Ulbrych <89152624+StanFromIreland@users.noreply.github.com>
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
Linking PEPs should be avoided in new documentation because they are historical documents that may not match the runtime. Considering there isn't more informatino to add, and I think, there shouldn't have, I suggest closing this PR.
What may be interesting though is to document when an exception is set by the function. PyModule_GetState returns NULL with an exception set if module is not a module, and returns NULL without an exception set if PyModuleDef.md_state is NULL which, AFAIK, only happens when memory cannot be allocated or m_size is 0:
- If it cannot be allocated, I'mnot even sure we would be able to get to the call of
PyModule_GetState. - If
m_sizeis 0, it means that the module state was really an empty object. So there is no state. And unless you callPyModule_GetStatefor a module you don't know the state of, then it won't happen. But again, if you call that function for a module you don't know the state of, it doesn't make sense to use it as you get avoid *.
Unless we want to document this subtlety, we should close this PR.
6 participants
This PR adds more information on how to use the
PyModule_GetStatefunction, clarifying what argument should be passed.At first glance, people might incorrectly think they need to pass a pointer to a state struct that the function will populate, but that is not the case.
📚 Documentation preview 📚: https://cpython-previews--138823.org.readthedocs.build/