Skip to content
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

Add the 'wrapper_descriptor' type to the types module #73563

Closed
wheerd mannequin opened this issue Jan 26, 2017 · 17 comments
Closed

Add the 'wrapper_descriptor' type to the types module #73563

wheerd mannequin opened this issue Jan 26, 2017 · 17 comments
Labels
3.7 stdlib type-feature

Comments

@wheerd
Copy link
Mannequin

@wheerd wheerd mannequin commented Jan 26, 2017

BPO 29377
Nosy @gvanrossum, @1st1, @ilevkivskyi, @wheerd
PRs
  • #926
  • #552
  • Files
  • 0001-Added-SlotWrapperType-and-MethodWrapperType-to-the-t.patch: Added a patch with the changes.
  • slot-wrapper-types.patch: Updated patch
  • slot-wrapper-types.patch
  • slot-wrapper-types.patch
  • slot-wrapper-types.patch
  • combined-patch.diff
  • combined-patch-full-total.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-02-01.19:07:29.286>
    created_at = <Date 2017-01-26.10:16:18.054>
    labels = ['3.7', 'type-feature', 'library']
    title = "Add the 'wrapper_descriptor' type to the types module"
    updated_at = <Date 2017-03-31.16:36:31.640>
    user = 'https://github.com/wheerd'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:31.640>
    actor = 'dstufft'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-02-01.19:07:29.286>
    closer = 'gvanrossum'
    components = ['Library (Lib)']
    creation = <Date 2017-01-26.10:16:18.054>
    creator = 'Wheerd'
    dependencies = []
    files = ['46420', '46425', '46427', '46429', '46455', '46457', '46477']
    hgrepos = []
    issue_num = 29377
    keywords = ['patch']
    message_count = 17.0
    messages = ['286304', '286306', '286311', '286330', '286335', '286337', '286341', '286343', '286383', '286489', '286499', '286572', '286599', '286604', '286675', '286676', '286688']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'python-dev', 'yselivanov', 'levkivskyi', 'Wheerd']
    pr_nums = ['926', '552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29377'
    versions = ['Python 3.7']

    @wheerd
    Copy link
    Mannequin Author

    @wheerd wheerd mannequin commented Jan 26, 2017

    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.

    @wheerd wheerd mannequin added stdlib type-feature labels Jan 26, 2017
    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jan 26, 2017

    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 python/typing#368

    @wheerd
    Copy link
    Mannequin Author

    @wheerd wheerd mannequin commented Jan 26, 2017

    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.

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jan 26, 2017

    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)

    @wheerd
    Copy link
    Mannequin Author

    @wheerd wheerd mannequin commented Jan 26, 2017

    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.

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jan 26, 2017

    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.

    @wheerd
    Copy link
    Mannequin Author

    @wheerd wheerd mannequin commented Jan 27, 2017

    Alright, I added some tests and tried it again with the patch.

    @wheerd
    Copy link
    Mannequin Author

    @wheerd wheerd mannequin commented Jan 27, 2017

    I created the last patch without commiting, so maybe this one will work properly with the revision tool.

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jan 27, 2017

    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.

    @wheerd
    Copy link
    Mannequin Author

    @wheerd wheerd mannequin commented Jan 30, 2017

    Okay, I added MethodDescriptorType to the types module and updated the docs. Hope this is okay now.

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Jan 30, 2017

    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?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Feb 1, 2017

    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?

    @wheerd
    Copy link
    Mannequin Author

    @wheerd wheerd mannequin commented Feb 1, 2017

    One question I was wondering about is whether those types should be checked by inspect.isroutine() as well.

    @ilevkivskyi
    Copy link
    Contributor

    @ilevkivskyi ilevkivskyi commented Feb 1, 2017

    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.

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Feb 1, 2017

    New changeset 3db1959c2c53 by Guido van Rossum in branch 'default':
    Issue bpo-29377: Add three new wrappers to types.py (Manuel Krebber).
    https://hg.python.org/cpython/rev/3db1959c2c53

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Feb 1, 2017

    Thanks Manuel and Ivan!

    @python-dev
    Copy link
    Mannequin

    @python-dev python-dev mannequin commented Apr 9, 2022

    New changeset 1947d33 by Guido van Rossum in branch 'master':
    Issue bpo-29377: Add three new wrappers to types.py (Manuel Krebber).
    1947d33

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants