-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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-122102: Fix/improve docs of descriptor-related tools in inspect
#122104
base: main
Are you sure you want to change the base?
gh-122102: Fix/improve docs of descriptor-related tools in inspect
#122104
Conversation
ec0e2e9
to
5e7ce51
Compare
inspect
inspect
Note: it may be worth to apply the documentation changes to all supported versions of Python (starting with 3.8). |
|
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 emphasize the terms getset
, you should do it for slot
as well when talking about slot
descriptors I think. Personally, I would simply consider getset as a common noun, so no need for having *getset*
or *properties*
as well.
Misc/NEWS.d/next/Documentation/2024-07-22-01-30-49.gh-issue-122102.Za48MH.rst
Outdated
Show resolved
Hide resolved
Does this rule apply also to documentation-only changes? |
OK, you convinced me. :) |
AFAIK, yes. @hugovk Do you confirm? |
8ee1d5a
to
2a11629
Compare
inspect
inspect.is{data,method}descriptor()
inspect.is{data,method}descriptor()
inspect
inspect
inspect
I decided to exclude the tests-related changes from this PR, and create a separate PR that includes them. Rationale: those two sets of changes may be desired to be backported to different sets of Python versions, as:
I am very sorry for the fuss with all this PR splitting (I should have thought about it earlier...). :-| |
Doc/library/inspect.rst
Outdated
Method descriptors that also pass any of the other tests mentioned above | ||
(:func:`isclass`, :func:`ismethod` or :func:`isfunction`) make this function |
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's a bit weird to say "mentioned above" and re-mention them. Either you mention them again (which I wouldn't suggest) or you just say "mentioned above".
Doc/library/inspect.rst
Outdated
descriptors and member descriptors. Note that for the latter two (which can | ||
be defined only at the C level, in extension modules) more specific tests | ||
are available: :func:`isgetsetdescriptor` and :func:`ismemberdescriptor`, | ||
respectively. |
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.
descriptors and member descriptors. Note that for the latter two (which can | |
be defined only at the C level, in extension modules) more specific tests | |
are available: :func:`isgetsetdescriptor` and :func:`ismemberdescriptor`, | |
respectively. | |
descriptors and member descriptors. Note that specific tests robust | |
across different Python implementations are available for the latter | |
two (defined in C extension modules), namely :func:`isgetsetdescriptor` | |
and :func:`ismemberdescriptor` respectively. |
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.
I find the fragment about those two specific tests being robust across different Python implementations quite problematic: in a way, those two tests can be considered less robust across different Python implementations. (If I needed a test returning true for, e.g., a frame's f_locals
regardless of the Python implementation being used, I'd use isdatadescriptor()
rather than isgetsetdescriptor()
...).
That's why I propose to remove that fragment.
IMHO, it causes more confusion than good.
Also I believe that the statement that those two tests are more specific, together with the docs of those two tests themselves, convey enough information.
Doc/library/inspect.rst
Outdated
Typically, data descriptors have also :attr:`~definition.__name__` and | ||
:attr:`!__doc__` attributes (properties, getsets and member descriptors have | ||
them), but this is not guaranteed. |
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.
Typically, data descriptors have also :attr:`~definition.__name__` and | |
:attr:`!__doc__` attributes (properties, getsets and member descriptors have | |
them), but this is not guaranteed. | |
While data descriptors such as properties, getsets or member descriptors | |
have :attr:`~definition.__name__` and :attr:`!__doc__` attributes, this | |
is not necessarily the case in general. |
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.
I believe that the fragment referring to properties, getsets or member descriptors should remain in parentheses, as it conveys here only additional (extra) information, not the crucial one (which is that, in general, the presence of the data descriptors' attributes __name__
and __doc__
is likely but optional).
:attr:`!__doc__` attributes (properties, getsets and member descriptors have | ||
them), but this is not guaranteed. | ||
|
||
.. versionchanged:: 3.8 |
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.
Was it really fixed since 3.8?
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, with the PR gh-1959.
Misc/NEWS.d/next/Documentation/2024-07-22-01-30-49.gh-issue-122102.Za48MH.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Bénédikt Tran <[email protected]>
…2102.Za48MH.rst Co-authored-by: Bénédikt Tran <[email protected]>
(further edits of docstrings)
Sorry, I just learned that docs-only changes should not be announced in NEWS. So I delete the blurb. |
Correct the docs of
ismethoddescriptor()
by removing the mistaken information about relation withisbuiltin()
.Complement the docs of
isdatadescriptor()
by adding the missing information about relations withisclass()
,ismethod()
andisfunction()
.Update and improve the docs (and docstrings) of those two functions.
📚 Documentation preview 📚: https://cpython-previews--122104.org.readthedocs.build/
inspect.ismethoddescriptor()
's andinspect.isdatadescriptor()
's docs and unit tests #122102