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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:42 | admin | set | github: 73563 |
2017-03-31 16:36:31 | dstufft | set | pull_requests:
+ pull_request1045 |
2017-03-31 12:28:43 | Jim Fasarakis-Hilliard | set | pull_requests:
+ pull_request823 |
2017-02-01 20:00:31 | python-dev | set | stage: resolved |
2017-02-01 20:00:30 | python-dev | set | messages:
+ msg286688 |
2017-02-01 19:07:29 | gvanrossum | set | status: open -> closed resolution: fixed messages:
+ msg286676
|
2017-02-01 19:05:59 | python-dev | set | nosy:
+ python-dev messages:
+ msg286675
|
2017-02-01 09:29:21 | levkivskyi | set | files:
+ combined-patch-full-total.diff
messages:
+ msg286604 |
2017-02-01 09:03:57 | Wheerd | set | messages:
+ msg286599 |
2017-02-01 00:37:15 | gvanrossum | set | messages:
+ msg286572 |
2017-01-30 22:43:03 | levkivskyi | set | files:
+ combined-patch.diff nosy:
+ yselivanov messages:
+ msg286499
|
2017-01-30 12:22:15 | Wheerd | set | files:
+ slot-wrapper-types.patch
messages:
+ msg286489 |
2017-01-27 20:56:29 | levkivskyi | set | messages:
+ msg286383 |
2017-01-27 08:11:11 | Wheerd | set | files:
+ slot-wrapper-types.patch
messages:
+ msg286343 |
2017-01-27 07:55:18 | Wheerd | set | files:
+ slot-wrapper-types.patch
messages:
+ msg286341 |
2017-01-26 23:06:13 | levkivskyi | set | messages:
+ msg286337 |
2017-01-26 22:15:59 | Wheerd | set | files:
+ slot-wrapper-types.patch
messages:
+ msg286335 |
2017-01-26 18:22:58 | levkivskyi | set | messages:
+ msg286330 |
2017-01-26 13:34:24 | Wheerd | set | messages:
+ msg286311 |
2017-01-26 13:26:29 | Wheerd | set | files:
+ 0001-Added-SlotWrapperType-and-MethodWrapperType-to-the-t.patch keywords:
+ patch |
2017-01-26 11:18:04 | levkivskyi | set | nosy:
+ gvanrossum, levkivskyi
messages:
+ msg286306 versions:
+ Python 3.7 |
2017-01-26 10:16:18 | Wheerd | create | |