◐ Shell
reader mode source ↗
Skip to content

bpo-32380 Fix singledispatch interaction with methods#4987

Closed
emmatyping wants to merge 5 commits into
python:masterfrom
emmatyping:singledispatch
Closed

bpo-32380 Fix singledispatch interaction with methods#4987
emmatyping wants to merge 5 commits into
python:masterfrom
emmatyping:singledispatch

Conversation

@emmatyping

@emmatyping emmatyping commented Dec 23, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Singledispatch dispatches on the first argument, making it incompatible with classes. This change fixes that.

https://bugs.python.org/issue32380

@emmatyping
bpo-32380 Fix singledispatch interaction with methods …
9351f1c 
Singledispatch used the first argument to dispatch on. This made it
incomatible with methods. This change adds support for methods by
dispatching on the second argument if the passed function is a method.
ilevkivskyi
ilevkivskyi requested changes Dec 23, 2017
View reviewed changes

@ilevkivskyi ilevkivskyi left a comment

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.

Hide comment

Thanks! I think this should be implemented more carefully, see comments.

@bedevere-bot

bedevere-bot commented Dec 23, 2017

Copy link
Copy Markdown

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.

ncoghlan
ncoghlan requested changes Dec 25, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

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.

Hide comment

Thanks for the PR, but unfortunately, I don't believe this particular class-definition-time-only approach is ever going to work right given the semantics of the descriptor protocol.

Instead, something more like the functools.partialmethod will be needed, which defers some key tasks to its __get__ method implementation.

@blag blag mentioned this pull request Jan 22, 2018
Closed
@emmatyping

emmatyping commented Jan 31, 2018

Copy link
Copy Markdown
Member Author

I'm afraid I don't have the bandwidth for this at the moment. @ianhoffman if you want to take over be my guest. I'll come back to it another time if not.

@emmatyping emmatyping closed this Jan 31, 2018
@ilevkivskyi

ilevkivskyi commented Feb 3, 2018

Copy link
Copy Markdown
Member

It is sad to see this closed, I think it would be a useful addition, please keep me posted if you will resume your work.

@emmatyping

emmatyping commented Feb 3, 2018

Copy link
Copy Markdown
Member Author

I want to focus on PEP 561 and a personal project, but I might get back to this after a month or two.

@pganssle pganssle mentioned this pull request Mar 4, 2018
Closed
@emmatyping emmatyping deleted the singledispatch branch March 15, 2018 09:04
@emmatyping emmatyping mentioned this pull request Mar 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ilevkivskyi ilevkivskyi ilevkivskyi requested changes

@ncoghlan ncoghlan ncoghlan requested changes

@rhettinger rhettinger Awaiting requested review from rhettinger

+1 more reviewer

@ianhoffman ianhoffman ianhoffman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@emmatyping @bedevere-bot @ilevkivskyi @ncoghlan @ianhoffman @the-knights-who-say-ni