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
Test: added more method/function cases (requested by @eendebakpt)
  • Loading branch information
@zuo
zuo committed Jun 15, 2024
commit 71c3735411f955fbe28e4b71956204bcb8fb06db
5 changes: 5 additions & 0 deletions Lib/test/test_inspect/test_inspect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1864,6 +1864,7 @@ class Slotted:
__slots__ = 'foo',
def function():
pass
a_lambda = lambda: None

# Example builtin method descriptors:
self.assertTrue(
Expand All @@ -1888,8 +1889,12 @@ def function():
# method descriptors, but `ismethoddescriptor()` explicitly
# excludes them):
self.assertFalse(inspect.ismethoddescriptor(Owner().instance_method))
self.assertFalse(inspect.ismethoddescriptor(Owner().class_method))
self.assertFalse(inspect.ismethoddescriptor(Owner.class_method))
self.assertFalse(inspect.ismethoddescriptor(Owner().static_method))

@eendebakpt eendebakpt Jun 13, 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.

Suggested change
self.assertFalse(inspect.ismethoddescriptor(Owner().static_method))
self.assertFalse(inspect.ismethoddescriptor(Owner().static_method))
self.assertFalse(inspect.ismethoddescriptor(Owner.instance_method))
self.assertFalse(inspect.ismethoddescriptor(Owner.class_method))
self.assertFalse(inspect.ismethoddescriptor(Owner.static_method))

@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.

Frankly, I'd rather prefer to reduce the number of these cases...

I'd keep:

  • Owner().instance_method (method object – bound to an instance),
  • Owner().class_method (method object – bound to a class),
  • Owner().static_method (just a function object)

I'd remove: function and a_lambda completely – each of them is also just a function object, so they are redundant.

When it comes to the ones you proposed, please note that Owner.instance_method and Owner.static_method are also just function objects, and Owner.class_method is a method object bound to a class (identical to the Owner().class_method one).

This test class should focus on the behavior of inspect.ismethoddescriptor(). Various ways of obtaining objects of the same type are out of scope of it.

(In the first version, I wanted to mimic some elements of the already existing test class TestIsDataDescriptor. But now I believe that that one should be improved instead – though rather not as a part of this PR).

@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.

Do you agree with my reasoning, or would you prefer these new cases to be added anyway? Could you please decide, I have no strong feelings about this particular detail. :)

@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.

I prefer having all possible cases. One reason is that it shows how robust the inspect is and for me (at Sphinx), it's quite important to have a robust inspect module. So, if there are some possibilities in the future where something could go wrong (hello Murphy), it's better to have a wide coverage.

@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.

OK, added :)

@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.

Thank you! (it also implicitly helps in asserting that a function and a lambda function have the same type)

self.assertFalse(inspect.ismethoddescriptor(Owner.static_method))
self.assertFalse(inspect.ismethoddescriptor(function))
self.assertFalse(inspect.ismethoddescriptor(a_lambda))

def test_descriptor_being_a_class(self):
class MethodDescriptorMeta(type):
Expand Down