Skip to content

gh-120381: Fix inspect.ismethoddescriptor() #120383

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.

Sign up for GitHub

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

Merged
ncoghlan merged 16 commits into from
Jun 18, 2024
Merged

gh-120381: Fix inspect.ismethoddescriptor() #120383

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
dcc561b
gh-120381: Fix inspect.ismethoddescriptor()
zuo Jun 12, 2024
7740b0e
Merge branch 'main' into inspect-ismethoddescriptor-fix
zuo Jun 12, 2024
2dc2168
Fix the blurb
zuo Jun 12, 2024
4f13243
Merge branch 'main' into inspect-ismethoddescriptor-fix
zuo Jun 12, 2024
7fdc95a
Merge branch 'main' into inspect-ismethoddescriptor-fix
zuo Jun 15, 2024
52f579f
Clean up/improve/refactor tests
zuo Jun 15, 2024
57875a7
Refactor
zuo Jun 15, 2024
01178fa
Doc: add `versionchanged`...
zuo Jun 15, 2024
71c3735
Test: added more method/function cases (requested by @eendebakpt)
zuo Jun 15, 2024
2daa16f
Test: added one more method/function case...
zuo Jun 15, 2024
0c7bb3f
Update Doc/library/inspect.rst
zuo Jun 15, 2024
84aed74
A couple of renames
zuo Jun 16, 2024
0390f85
Improve the `versionchanged` description
zuo Jun 16, 2024
bcfeeb6
Update Doc/library/inspect.rst
zuo Jun 16, 2024
eb4d852
minor reST syntax fix
zuo Jun 16, 2024
8ccb761
Merge branch 'main' into inspect-ismethoddescriptor-fix
ncoghlan Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Doc: add versionchanged...
  • Loading branch information
@zuo
zuo committed Jun 15, 2024
commit 01178fab7a0f50537e8aa83eef830f03ad8dd467
6 changes: 6 additions & 0 deletions Doc/library/inspect.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,12 @@ attributes (see :ref:`import-mod-attrs` for module attributes):
:attr:`~method.__func__` attribute (etc) when an object passes
:func:`ismethod`.

.. versionchanged:: 3.13
For objects with :meth:`~object.__get__` and :meth:`~object.__delete__`
but without :meth:`~object.__set__`, ``False`` is now returned
(previously, ``True`` was returned for such objects, which was
incorrect).

@picnixz picnixz Jun 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For objects with :meth:`~object.__get__` and :meth:`~object.__delete__`
but without :meth:`~object.__set__`, ``False`` is now returned
(previously, ``True`` was returned for such objects, which was
incorrect).
Objects with :meth:`~object.__get__` and :meth:`~object.__delete__`
but without :meth:`~object.__set__` are no more considered as method
descriptors. In previous versions, they were incorrectly deemed as such.

@zuo zuo Jun 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@picnixz OK, I committed your suggestion. However, doesn't the fragment are no more considered as method descriptors sound too general? I mean, someone could misunderstand it – that before Python 3.13 such objects were generally considered as method descriptors; whereas our intent is to inform that just this particular function incorrectly considered them as such.

What do you think about such wording:

Objects with __get__() and __delete__() but without __set__() are no more considered by this function as method descriptors. In previous versions, they incorrectly were.

@zuo zuo Jun 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Or some other wording that would emphasize the focus on the behavior of this particular function...)

@zuo zuo Jun 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just committed as slightly different variant:

      Objects with :meth:`~object.__get__` and :meth:`~object.__delete__`
      but without :meth:`~object.__set__` are no more considered by this
      function as method descriptors (in previous versions, they were
      incorrectly deemed as such).

Could you please decide whether it is OK. :)

@ncoghlan ncoghlan Jun 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a suggestion to tighten this up and make it clear it is describing a bug fix in this function rather than a change to the language itself.

@zuo zuo Jun 16, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! :) I applied it.

@picnixz picnixz Jun 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thanks! my suggestions are in the end only... suggestions. I appreciate when someone likes it verbatim but I don't mind when it is reformulated!



.. function:: isdatadescriptor(object)

Expand Down