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

bpo-32380: Create functools.singledispatchmethod #6306

Merged
merged 8 commits into from May 26, 2018

Conversation

Projects
None yet
6 participants
@ethanhs
Copy link
Contributor

commented Mar 29, 2018

I finally found time to follow up after #4987. In review: functools.singledispatch interacts poorly with methods and descriptors, thus I created functools.singledispatchmethod to handles these.

This implementation is based on that of functools.partialmethod.

https://bugs.python.org/issue32380

bpo-32380: Create functools.singledispatchmethod
functools.singledispatch interacts poorly with methods and descriptors,
thus functools.singledispatchmethod is created to handles these.
@ethanhs

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2018

Hm, I'm not sure what is going on with the unexpected indent here. It seems valid.

@ambv
Copy link
Contributor

left a comment

This looks promising, thanks for working on it.

@@ -2147,6 +2147,73 @@ def __eq__(self, other):
return self.arg == other
self.assertEqual(i("str"), "str")

def test_method_register(self):

This comment has been minimized.

Copy link
@ambv

ambv Mar 29, 2018

Contributor

I don't see tests that show that registration of arbitrary callables from outside of the class definition also works.

Something like:

class C:
  @singledispatchmethod
  def g(self, arg):
    ...

@C.g.register(int)
def _(self, arg):
  ...
raise NotImplementedError("Cannot negate a")

@neg.register(int):
def _(self, arg):

This comment has been minimized.

Copy link
@ambv

ambv Mar 29, 2018

Contributor

Please document using type annotations as the default way to register types, just like the singledispatch documentation in 3.7+ does.

def __init__(self, func):
if not callable(func) and not hasattr(func, "__get__"):
raise TypeError("{!r} is not callable or a descriptor"
.format(func))

This comment has been minimized.

Copy link
@ambv

ambv Mar 29, 2018

Contributor

nit: this formatting is ugly. Consider:

raise TypeError(
    f"{func!r} is not callable or a descriptor"
)

Might even fit in a single line with the f-string.

@bedevere-bot

This comment has been minimized.

Copy link

commented Mar 29, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

like :func:`singledispatch` except that it is designed to be used as a
method definition rather than being directly callable.

:class:`singledispatchmethod` can be used as follows:

This comment has been minimized.

Copy link
@ambv

ambv Mar 29, 2018

Contributor

The unexpected indent is likely because you didn't use double colon (::) here. But I don't know if the empty line doesn't denote an end of the code block in RST. Look for other examples where stuff like this is used in the docs, they will probably hint at a good solution.

ethanhs added some commits Mar 30, 2018

@ethanhs

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

I have made the requested changes; please review again

Thanks for the review Łukasz!

@bedevere-bot

This comment has been minimized.

Copy link

commented Mar 30, 2018

Thanks for making the requested changes!

@ambv: please review the changes made to this pull request.

@ncoghlan
Copy link
Contributor

left a comment

Some documentation suggests inline, but I otherwise don't have anything to add to @ambv's comments.

Nice work on remembering to test the interactions with other standard library decorators!

@@ -383,6 +383,28 @@ The :mod:`functools` module defines the following functions:
The :func:`register` attribute supports using type annotations.


.. class:: singledispatchmethod(func)

Return a new :class:`singledispatchmethod` descriptor which behaves

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 30, 2018

Contributor

While this is implemented as a class, I'd suggest borrowing the markup and introductory text from singledispatch and reusing an adjusted version of that text here - from an end user's perspective, the important point is that they can use singledispatchmethod to decorate a method definition and have it dispatch on the first non-self argument.

@neg.register
def _(self, arg: bool):
return not arg

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 30, 2018

Contributor

Having an example stacked with @classmethod would be good here. It's unfortunate that the order of application will need to be different from that when stacking @abstractmethod (see https://docs.python.org/3/library/abc.html#abc.abstractmethod), but we need "register" to be accessible on the resulting descriptor. So something like:

class Negator:
    @singledispatchmethod
    @classmethod
    def neg(cls, arg):
        ...

Given the @classmethod example, you can then note that the same principle applies for @staticmethod and other decorators - to be useful, @singledispatchmethod typically needs to be the outermost decorator so that the register method is accessible.

@ethanhs

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

@ncoghlan thanks for the suggestions! I amended the docs as you asked.

@ambv
Copy link
Contributor

left a comment

This is almost there. Two remaining requests:

  • in the Negator example in the docs, use cls, not self
  • in the tests, use registered implementations which actually use the relevant cls or self to prove they bind correctly; especially in the arbitrary callable case.

ethanhs added some commits Mar 30, 2018

@ethanhs

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

This is almost there. Two remaining requests:

  • in the Negator example in the docs, use cls, not self
  • in the tests, use registered implementations which actually use the relevant cls or self to prove they >bind correctly; especially in the arbitrary callable case.

Done! Let me know if you have further feedback.

@ambv

ambv approved these changes Mar 30, 2018

@ambv

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2018

I'll wait a few days before merging in case @rhettinger has any input.

@ilevkivskyi

This comment has been minimized.

Copy link
Contributor

commented May 25, 2018

What is the status here? This seems ready to be merged. (Or maybe it should also support annotations like @singledispatch does now ;-))

@ethanhs

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2018

@ilevkivskyi waiting on naming approval. Also, It does support annotations see https://github.com/python/cpython/pull/6306/files#diff-09cc1e5ad87c6bd130e5b417193ed883R2251

@ambv ambv merged commit c651275 into python:master May 26, 2018

4 checks passed

bedevere/issue-number Issue number 32380 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bedevere-bot

This comment has been minimized.

Copy link

commented May 26, 2018

@ambv: Please replace # with GH- in the commit message next time. Thanks!

@ambv

This comment has been minimized.

Copy link
Contributor

commented May 26, 2018

Thanks! 🍰

lisroach added a commit to lisroach/cpython that referenced this pull request Sep 10, 2018

yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.