bpo-29916: Include PyGetSetDef in C API extension documentation #831
Conversation
|
@MSeifert04, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @ashemedai and @vadmium to be potential reviewers. |
|
Please open an issue on the bug tracker. |
done |
Doc/c-api/structures.rst
Outdated
| +-------------+------------------+-----------------------------------+ | ||
| | Field | C Type | Meaning | | ||
| +=============+==================+===================================+ | ||
| | name | char \* | attribute name. | |
There was a problem hiding this comment.
const char \*
See #846.
Doc/c-api/structures.rst
Outdated
| +-------------+------------------+-----------------------------------+ | ||
|
|
||
|
|
||
| .. c:type:: getter |
There was a problem hiding this comment.
Are any other names without the "Py" prefix documented? If are not, I would prefer to not document getter and setter explicitly.
There was a problem hiding this comment.
There are several definitions METH_VARARGS, etc. without the Py_-prefix. I just "copied" that "style".
There was a problem hiding this comment.
You decide, drop or keep? And if dropped should the explanation stay or be deleted as well?
There was a problem hiding this comment.
I have opened a thread on Python-Dev. If the decision will be to add Py-prefixed aliases, they can be documented as common types. Otherwise I would prefer to not document them explicitly (as well as a number of other typedefs in typeobj.rst).
Doc/c-api/structures.rst
Outdated
| typedef int (*setter)(PyObject *, PyObject *, void *); | ||
|
|
||
| In case the attribute should be deleted the second parameter is *NULL*. | ||
| Should return 0 on success or -1 with a set exception on failure. |
There was a problem hiding this comment.
Format return codes as ``0`` and ``-1``.
|
I removed the The updated documentation (preview) looks like this:
|
|
@MSeifert04 Could you add the issue number to the title of your PR? E.g. "bpo-1234: include ...". |
MSeifert04
changed the title
|
@brettcannon, you can edit the title yourself. |
|
done |
|
@serhiy-storchaka I know, but no comment was left here in the PR of what the actual issue number was, so it was just as easy to ask @MSeifert04 to add the issue number instead of ask for the number and then me manually adding the number (especially if @MSeifert04 contributes again in the future). |
|
FYI: The first commit did contain the issue number. But I forgot to change the title after changing the commit message :) |
|
Anything I can do here? Or do I just wait for someone to review (proofread) it? |
|
Just wait until we decide what to do with names like |
|
I just noticed that several of these |
|
There haven't been any new answers to the python-dev thread. Does that mean the PR is essentially declined or just on hold? |
brettcannon
changed the title
|
Part of the delay at this point is last week a discussion started about trying to clean up the C API. But I think at this point we probably don't need to wait for prefixing since if we start cleaning up things we will add the aliases then. What do you think @serhiy-storchaka ? And please ignore the opening/closing as that was just an attempt to trigger the issue number status check. |
|
I don't know. I asked on Python-Dev but didn't get an answer. |
|
So this will just stay open/unmerged until a consensus (on a topic nobody is discussing) is reached? I thought this PR was just a minor documentation improvement but I don't like the fact that it has completely stalled. If there's no interest in this I'll gladly close the PR and move on... |
Doc/c-api/structures.rst
Outdated
| +-------------+------------------+-----------------------------------+ | ||
| | Field | C Type | Meaning | | ||
| +=============+==================+===================================+ | ||
| | name | const char \* | attribute name. | |
There was a problem hiding this comment.
If the description ends with a period, it should starts with an upper case. But descriptions in other tables don't start with an upper case and don't contain periods. Please make the style of this table matching the style of other tables.
The only problem is with the set description, which consists of two sentences. I think you can use a semicolon for separating two parts.
Doc/c-api/structures.rst
Outdated
| The ``get`` function takes one :c:type:`PyObject\*` parameter (the | ||
| instance) and a function pointer (the associated ``closure``):: | ||
|
|
||
| typedef PyObject *(*getter)(PyObject *, void *); |
There was a problem hiding this comment.
Please remove one space before typedef. Three-space indentation is used in this file.
|
Thank you for a reminder and sorry for the delay. Your change definitely is an improvement. We can change the documentation for getter and setter in other issue. LGTM, except two nitpicks. |
|
Thanks for the review. I changed it accordingly. There is a failure due to a missing news item or missing label. I assume this doesn't warrant a news-item so I think the label should be attached but let me know if I need to add a news-entry. |
|
Thanks @MSeifert04 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
|
Thanks @MSeifert04 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
Sorry, @MSeifert04 and @serhiy-storchaka, I could not cleanly backport this to |
|
GH-3607 is a backport of this pull request to the 3.6 branch. |
|
Thanks @MSeifert04 for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7. |
|
Sorry, @MSeifert04 and @serhiy-storchaka, I could not cleanly backport this to |
|
GH-3609 is a backport of this pull request to the 2.7 branch. |
There is currently only indirect documentation of the
PyGetSetDefstruct in the documentation in thePyTypeObject.tp_getsetslot. However the documentation there contains a note that it "belongs elsewhere".The rendered documentation looks like this:
https://bugs.python.org/issue29916