GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-33967: singledispatch raises TypeError when no positional arguments by corona10 · Pull Request #8184 · python/cpython

Conversation

@corona10

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue33967

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
corona10 requested review from ncoghlan and rhettinger as code owners July 8, 2018 09:51
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 8, 2018
@bedevere-bot bedevere-bot added the awaiting review label Jul 8, 2018
@methane

methane commented Jul 9, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I think this is too much.

I think this is enough:

if not args:
    raise TypeError("singledispatch requires at least 1 positional argument")
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@methane
methane requested a review from ambv July 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented Jul 9, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Okay can I update this PR into suggested one?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 force-pushed the bpo-33967 branch from 4f72f83 to 0962fd3 Compare July 9, 2018 11:28
@corona10

corona10 commented Jul 9, 2018

Copy link
Copy Markdown
Member Author

@doerwalter @methane

Updated! Please take a look!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
methane
methane reviewed Jul 9, 2018
View reviewed changes
Comment thread Lib/test/test_functools.py Outdated

@methane methane Jul 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
corona10 force-pushed the bpo-33967 branch from 0962fd3 to 7aad76b Compare July 9, 2018 12:00
@corona10

corona10 commented Jul 9, 2018

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@methane methane changed the title bpo-33967: Improve functools.singledispatch exception messages bpo-33967: singledispatch raises TypeError when no positional arguments Jul 9, 2018
methane
methane reviewed Jul 9, 2018
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2018-07-08-18-49-41.bpo-33967.lhaAez.rst Outdated

@methane methane Jul 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka and corona10 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@methane methane added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Jul 9, 2018
@corona10
corona10 force-pushed the bpo-33967 branch from 7aad76b to 26d4234 Compare July 9, 2018 13:45
@corona10

corona10 commented Jul 9, 2018

Copy link
Copy Markdown
Member Author

@methane I've updated news
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Jul 9, 2018
View reviewed changes
Comment thread Lib/functools.py Outdated

@serhiy-storchaka serhiy-storchaka Jul 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Jul 9, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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')
def wrapper(*args, **kw):
    # use funcname

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@corona10
bpo-33967: functools.singledispatch now raises TypeError
8730dbd
@corona10
corona10 force-pushed the bpo-33967 branch from 26d4234 to 8730dbd Compare July 10, 2018 02:15
@corona10

corona10 commented Jul 10, 2018

Copy link
Copy Markdown
Member Author

I fixed it
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Jul 10, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Jul 10, 2018
@methane
methane merged commit 445f1b3 into python:master Jul 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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 10, 2018
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Jul 10, 2018
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 <[email protected]>
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.6 label Jul 10, 2018
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
ZackerySpytz
ZackerySpytz reviewed Jul 12, 2018
View reviewed changes
Comment thread Lib/test/test_functools.py
def f(*args):
pass
msg = 'f requires at least 1 positional argument'
with self.assertRaisesRegexp(TypeError, msg):

@ZackerySpytz ZackerySpytz Jul 12, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
corona10 deleted the bpo-33967 branch November 22, 2018 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@ncoghlan ncoghlan Awaiting requested review from ncoghlan
@rhettinger rhettinger Awaiting requested review from rhettinger
@ambv ambv Awaiting requested review from ambv
+1 more reviewer
@ZackerySpytz ZackerySpytz ZackerySpytz left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@corona10 @methane @doerwalter @miss-islington @bedevere-bot @serhiy-storchaka @ZackerySpytz @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.