Skip to content

bpo-33967: singledispatch raises TypeError when no positional arguments - #8184

Merged
methane merged 1 commit into
python:masterfrom
corona10:bpo-33967
Jul 10, 2018
Merged

bpo-33967: singledispatch raises TypeError when no positional arguments#8184
methane merged 1 commit into
python:masterfrom
corona10:bpo-33967

Conversation

@corona10

@corona10corona10 commented Jul 8, 2018•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@methane

methane commented Jul 9, 2018•
edited
Loading

Copy link
Copy Markdown
Member

I think this is too much.

I think this is enough:

ifnotargs:
raiseTypeError("singledispatch requires at least 1 positional argument")

@methane
methane requested a review from ambvJuly 9, 2018 09:15
@doerwalter

doerwalter commented Jul 9, 2018

Copy link
Copy Markdown
Contributor

True, both f(x=3) and g(*args)can be called with one argument, so shouldn't be rejected. I think @methane's solution does the right thing.

@corona10

corona10 commented Jul 9, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Okay can I update this PR into suggested one?

@corona10

corona10 commented Jul 9, 2018

Copy link
Copy Markdown
MemberAuthor

@doerwalter@methane

Updated! Please take a look!

methane
methane reviewed Jul 9, 2018
View reviewed changes
Comment threadLib/test/test_functools.py Outdated

@methanemethaneJul 9, 2018

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.

Test seems too redundant for now.
TypeError happens always when no positional arguments.
How the generic function defined is not important.

@methane

methane commented Jul 9, 2018

Copy link
Copy Markdown
Member

f"{func.__name__} requires at least 1 positional argument" may be better, if we can assume func.__name__ is available always.

I believe singledispatch requires positional argument by design, intentionally.
But I want @ambv review before merge.

@corona10

corona10 commented Jul 9, 2018

Copy link
Copy Markdown
MemberAuthor

@methane
Thanks for the quick review.
Updated!
One more thing, we need to add tags needs backport to 3.7 / 3.6.

@methanemethane changed the title bpo-33967: Improve functools.singledispatch exception messagesbpo-33967: singledispatch raises TypeError when no positional argumentsJul 9, 2018
methane
methane reviewed Jul 9, 2018
View reviewed changes
Comment threadMisc/NEWS.d/next/Library/2018-07-08-18-49-41.bpo-33967.lhaAez.rst Outdated

@methanemethaneJul 9, 2018

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.

Error message is not important here. (We won't backport such improvements)

functools.singledispatch now raises TypeError instead of IndexError when no
positional arguments are passed.

@methanemethane added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Jul 9, 2018
@corona10

corona10 commented Jul 9, 2018

Copy link
Copy Markdown
MemberAuthor

@methane I've updated news

serhiy-storchaka
serhiy-storchaka reviewed Jul 9, 2018
View reviewed changes
Comment threadLib/functools.py Outdated

@serhiy-storchakaserhiy-storchakaJul 9, 2018

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.

func can be not having the __name__ attribute in general case. This PR can break a code which use singledispatch() with custom callables.

@serhiy-storchakaserhiy-storchakaJul 9, 2018

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.

Also this change creates a new reference to func linked to from wrapper. It may be better to keep a reference just to the name. E.g.

funcname=getattr(func, '__name__', 'singledispatch function')
defwrapper(*args, **kw):
# use funcname

@corona10

corona10 commented Jul 10, 2018

Copy link
Copy Markdown
MemberAuthor

I fixed it

@methane
methane merged commit 445f1b3 into python:masterJul 10, 2018
@miss-islington

miss-islington commented Jul 10, 2018

Copy link
Copy Markdown
Contributor

Thanks @corona10 for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

@bedevere-bot

bedevere-bot commented Jul 10, 2018

Copy link
Copy Markdown

GH-8220 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 10, 2018
@corona10@miss-islington
bpo-33967: Fix singledispatch raised IndexError when no args (pythonG…
74b26a1
…H-8184)
(cherry picked from commit 445f1b3)
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@bedevere-bot

bedevere-bot commented Jul 10, 2018

Copy link
Copy Markdown

GH-8221 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 10, 2018
@corona10@miss-islington
bpo-33967: Fix singledispatch raised IndexError when no args (pythonG…
a09058c
…H-8184)
(cherry picked from commit 445f1b3)
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
miss-islington added a commit that referenced this pull request Jul 10, 2018
@miss-islington@corona10
bpo-33967: Fix singledispatch raised IndexError when no args (GH-8184)
df9f633
(cherry picked from commit 445f1b3)
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
miss-islington added a commit that referenced this pull request Jul 10, 2018
@miss-islington@corona10
bpo-33967: Fix singledispatch raised IndexError when no args (GH-8184)
6ceab46
(cherry picked from commit 445f1b3)
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
ZackerySpytz
ZackerySpytz reviewed Jul 12, 2018
View reviewed changes
Comment threadLib/test/test_functools.py
def f(*args):
pass
msg = 'f requires at least 1 positional argument'
with self.assertRaisesRegexp(TypeError, msg):

@ZackerySpytzZackerySpytzJul 12, 2018

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.

assertRaisesRegexp() is deprecated, so this test fails when test_functools is run with -Werror (see #8261).

@corona10
corona10 deleted the bpo-33967 branch November 22, 2018 11:21
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methanemethanemethane left review comments
@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes
@ncoghlanncoghlanAwaiting requested review from ncoghlan
@rhettingerrhettingerAwaiting requested review from rhettinger
@ambvambvAwaiting requested review from ambv
+1 more reviewer
@ZackerySpytzZackerySpytzZackerySpytz left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type-bugAn unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@corona10@methane@doerwalter@miss-islington@bedevere-bot@serhiy-storchaka@ZackerySpytz@the-knights-who-say-ni