FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-30534: Fix error messages when pass keyword arguments by serhiy-storchaka · Pull Request #1901 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-30534: Fix error messages when pass keyword arguments - #1901

Merged
serhiy-storchaka merged 5 commits into
python:masterfrom
serhiy-storchaka:call-no-keyword-args
Jun 6, 2017
ConversationCommits (5)ChecksFiles changed
Merged

bpo-30534: Fix error messages when pass keyword arguments#1901
serhiy-storchaka merged 5 commits into
python:masterfrom
serhiy-storchaka:call-no-keyword-args

Conversation

serhiy-storchaka commented Jun 1, 2017

Copy link
Copy Markdown
Member

to functions implemented in C that don't support this.

bpo-30534: Fix error messages when pass keyword arguments
838d2ab 
to functions implemented in C that don't support this.
serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Jun 1, 2017
serhiy-storchaka requested a review from vstinner June 1, 2017 07:38
vstinner approved these changes Jun 1, 2017
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, I also like included cleanup changes ;-)

On the issue, I requested unit tests. But I also proposed to change the error message, so we write tests later, when we agree on what should be done.

vstinner reviewed Jun 6, 2017
View reviewed changes
Comment thread Lib/test/test_call.py

def test_varargs0_kw(self):
self.assertRaises(TypeError, {}.__contains__, x=2)

vstinner Jun 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You may keep this test for other Python implementations. (This test doesn't check the error message.)

serhiy-storchaka Jun 6, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a duplicate of the following test.

vstinner Jun 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh ok, you are right.

Comment thread Lib/test/test_itertools.py Outdated
except TypeError as err:
# we expect type errors because of wrong argument count
self.assertNotIn("does not take keyword arguments", err.args[0])
print(repr(err))

vstinner Jun 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Remove print()?

serhiy-storchaka Jun 6, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

Comment thread Objects/call.c Outdated
break;

case METH_VARARGS:
if (!(flags & METH_KEYWORDS) && nkwargs) {

vstinner Jun 6, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is "!(flags & METH_KEYWORDS)" still needed?

serhiy-storchaka Jun 6, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not needed. Removed.

vstinner approved these changes Jun 6, 2017
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

serhiy-storchaka merged commit 5eb788b into python:master Jun 6, 2017
serhiy-storchaka deleted the call-no-keyword-args branch June 6, 2017 15:45

serhiy-storchaka commented Jun 6, 2017

Copy link
Copy Markdown
Member Author

Thank you for your review @Haypo.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes

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.

3 participants

Back | FazBrowse Home | New Git URL