classification
Title: Add the 'wrapper_descriptor' type to the types module
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Wheerd, gvanrossum, levkivskyi, python-dev, yselivanov
Priority: normal Keywords: patch

Created on 2017-01-26 10:16 by Wheerd, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
0001-Added-SlotWrapperType-and-MethodWrapperType-to-the-t.patch Wheerd, 2017-01-26 13:26 Added a patch with the changes.
slot-wrapper-types.patch Wheerd, 2017-01-26 22:15 Updated patch
slot-wrapper-types.patch Wheerd, 2017-01-27 07:55
slot-wrapper-types.patch Wheerd, 2017-01-27 08:11 review
slot-wrapper-types.patch Wheerd, 2017-01-30 12:22
combined-patch.diff levkivskyi, 2017-01-30 22:43 review
combined-patch-full-total.diff levkivskyi, 2017-02-01 09:29 review
Pull Requests
URL Status Linked Edit
PR 926 merged Jim Fasarakis-Hilliard, 2017-03-31 12:28
PR 552 closed dstufft, 2017-03-31 16:36
Messages (17)
msg286304 - (view) Author: Manuel Krebber (Wheerd) * Date: 2017-01-26 10:16
There currently is no type in the types module for the slot wrappers/wrapper_descriptor types.

I would like to have something like

    WrapperDescriptor = type(object.__init__)

added to it (or maybe even add it to MethodType). This would be helpful to check more easily if some object is a method.

I can create a pull request for this if desired.
msg286306 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-01-26 11:18
Manuel, it would be helpful if you submit a patch.

Guido, this is necessary for ``get_type_hints`` to work nicer with built-in methods, see https://github.com/python/typing/pull/368
msg286311 - (view) Author: Manuel Krebber (Wheerd) * Date: 2017-01-26 13:34
I would suggest the names SlotWrapperType and MethodWrapperType because I think they match their string representations the closest.

For checking whether something is a method/function one could also use inspect.isroutine (or inspect.ismethoddescriptor), but that is inconsistent with regards to builtin and python methods:

>>> import inspect
>>> inspect.isroutine(object.__init__)
True
>>> inspect.isroutine(object().__init__)
False
>>> class A:
...   def f(self):
...     pass
...
>>> inspect.isroutine(A.f)
True
>>> inspect.isroutine(A().f)
True

Maybe a function to detect the second case is needed.
msg286330 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-01-26 18:22
Manuel, thank you for a patch!

Two comments:
1. Please produce your patch using Mercurial ``hg diff`` command, so that it could be recognized by review tool and merged easily.
2. Your patch should also include few tests (Lib/test/test_types.py) and documentation (Doc/library/types.rst)
msg286335 - (view) Author: Manuel Krebber (Wheerd) * Date: 2017-01-26 22:15
I added some docs, but I am not sure what I would want to test here. There are no tests for types.BuiltinMethodType either. Maybe the string representation of it could be tested, but an isinstance test seems pretty redundant. I hope this patch file works better, I created the last one with git diff.
msg286337 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-01-26 23:06
> but an isinstance test seems pretty redundant.

Tests are never redundant :-) Just add one-two asserts that you think should be true about issubclass and isinstance with these types (like self.assertIsInstance(''.__add__, types.MethodWrapperType)). String representation on the contrary is less important.

For some reason your patch is still not recognized by review tool. But don't worry about this, if it will not work, I will try to fix it.
msg286341 - (view) Author: Manuel Krebber (Wheerd) * Date: 2017-01-27 07:55
Alright, I added some tests and tried it again with the patch.
msg286343 - (view) Author: Manuel Krebber (Wheerd) * Date: 2017-01-27 08:11
I created the last patch without commiting, so maybe this one will work properly with the revision tool.
msg286383 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-01-27 20:56
Manuel, thank you for the new patch now everything works. I have few more comments:
1. I have found that there is one more built-in type: type(str.join) gives <class 'method_descriptor'>. Maybe it makes sense to add this one too?
2. Taking into account previous point I withdraw my idea of needing tests here. Sorry for changing opinion twice, but it looks like there are many built-in types that seem to exist just for historical reasons. They might change in future.
3. Please take a look at my review on documentation in Rietveld.
msg286489 - (view) Author: Manuel Krebber (Wheerd) * Date: 2017-01-30 12:22
Okay, I added MethodDescriptorType to the types module and updated the docs. Hope this is okay now.
msg286499 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-01-30 22:43
Thank you!
The new patch LGTM.
(I combined two diffs in your patch into one so that it could be understood by Rietveld).

Guido, Yury, could one of you please take a look at this?
msg286572 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2017-02-01 00:37
Seems the combined patch doesn't include the tests. Nor does the most recent slot-wrappre-types.patch.

Ivan, can you make a single truly combined patch and add a Misc/NEWS entry to it as well?

Other than that this looks fine.

Maybe we need to wait for the github migration to complete though?
msg286599 - (view) Author: Manuel Krebber (Wheerd) * Date: 2017-02-01 09:03
One question I was wondering about is whether those types should be checked by inspect.isroutine() as well.
msg286604 - (view) Author: Ivan Levkivskyi (levkivskyi) * Date: 2017-02-01 09:29
Guido, I attach the full patch now, as you asked.

(Initially I was not sure about tests, but now I understand more these types, so that I added even few more tests than in original patch)

> Maybe we need to wait for the github migration to complete though?

I think it is OK to merge this now, but if it would be easier for you then it is not a problem to wait until GH migration is complete.

Manuel, I think that probably the answer is yes, but I would prefer a separate issue for the inspect module. You could open a new issue and mention this one as a dependency.
msg286675 - (view) Author: Roundup Robot (python-dev) Date: 2017-02-01 19:05
New changeset 3db1959c2c53 by Guido van Rossum in branch 'default':
Issue #29377: Add three new wrappers to types.py (Manuel Krebber).
https://hg.python.org/cpython/rev/3db1959c2c53
msg286676 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2017-02-01 19:07
Thanks Manuel and Ivan!
msg286688 - (view) Author: Roundup Robot (python-dev) Date:
New changeset 1947d33a0a1c2ba006397579ec6235528faea9fd by Guido van Rossum in branch 'master':
Issue #29377: Add three new wrappers to types.py (Manuel Krebber).
https://github.com/python/cpython/commit/1947d33a0a1c2ba006397579ec6235528faea9fd
History
Date User Action Args
2017-03-31 16:36:31dstufftsetpull_requests: + pull_request1045
2017-03-31 12:28:43Jim Fasarakis-Hilliardsetpull_requests: + pull_request823
2017-02-01 20:00:31python-devsetstage: resolved
2017-02-01 20:00:30python-devsetmessages: + msg286688
2017-02-01 19:07:29gvanrossumsetstatus: open -> closed
resolution: fixed
messages: + msg286676
2017-02-01 19:05:59python-devsetnosy: + python-dev
messages: + msg286675
2017-02-01 09:29:21levkivskyisetfiles: + combined-patch-full-total.diff

messages: + msg286604
2017-02-01 09:03:57Wheerdsetmessages: + msg286599
2017-02-01 00:37:15gvanrossumsetmessages: + msg286572
2017-01-30 22:43:03levkivskyisetfiles: + combined-patch.diff
nosy: + yselivanov
messages: + msg286499

2017-01-30 12:22:15Wheerdsetfiles: + slot-wrapper-types.patch

messages: + msg286489
2017-01-27 20:56:29levkivskyisetmessages: + msg286383
2017-01-27 08:11:11Wheerdsetfiles: + slot-wrapper-types.patch

messages: + msg286343
2017-01-27 07:55:18Wheerdsetfiles: + slot-wrapper-types.patch

messages: + msg286341
2017-01-26 23:06:13levkivskyisetmessages: + msg286337
2017-01-26 22:15:59Wheerdsetfiles: + slot-wrapper-types.patch

messages: + msg286335
2017-01-26 18:22:58levkivskyisetmessages: + msg286330
2017-01-26 13:34:24Wheerdsetmessages: + msg286311
2017-01-26 13:26:29Wheerdsetfiles: + 0001-Added-SlotWrapperType-and-MethodWrapperType-to-the-t.patch
keywords: + patch
2017-01-26 11:18:04levkivskyisetnosy: + gvanrossum, levkivskyi

messages: + msg286306
versions: + Python 3.7
2017-01-26 10:16:18Wheerdcreate