Skip to content

bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords #916

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 9, 2017
Merged

bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60405e8
bpo-29951: Include function name for some error messages in PyArg_Par…
MSeifert04 Mar 30, 2017
5330e2c
bpo-29951: Include function name for PyArg_ParseTupleAndKeywords errors
MSeifert04 Mar 31, 2017
e4f4c24
bpo-29951: Changed exception message for missing required argument
MSeifert04 Mar 31, 2017
4da0e03
bpo-29551: undo the "keyword argument" part of the change
MSeifert04 Mar 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Lib/test/test_capi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,19 +533,19 @@ def test_positional_only(self):
parse((1, 2, 3), {}, b'OOO', ['', '', 'a'])
parse((1, 2), {'a': 3}, b'OOO', ['', '', 'a'])
with self.assertRaisesRegex(TypeError,
r'Function takes at least 2 positional arguments \(1 given\)'):
r'function takes at least 2 positional arguments \(1 given\)'):
parse((1,), {'a': 3}, b'OOO', ['', '', 'a'])
parse((1,), {}, b'O|OO', ['', '', 'a'])
with self.assertRaisesRegex(TypeError,
r'Function takes at least 1 positional arguments \(0 given\)'):
r'function takes at least 1 positional arguments \(0 given\)'):
parse((), {}, b'O|OO', ['', '', 'a'])
parse((1, 2), {'a': 3}, b'OO$O', ['', '', 'a'])
with self.assertRaisesRegex(TypeError,
r'Function takes exactly 2 positional arguments \(1 given\)'):
r'function takes exactly 2 positional arguments \(1 given\)'):
parse((1,), {'a': 3}, b'OO$O', ['', '', 'a'])
parse((1,), {}, b'O|O$O', ['', '', 'a'])
with self.assertRaisesRegex(TypeError,
r'Function takes at least 1 positional arguments \(0 given\)'):
r'function takes at least 1 positional arguments \(0 given\)'):
parse((), {}, b'O|O$O', ['', '', 'a'])
with self.assertRaisesRegex(SystemError, r'Empty parameter name after \$'):
parse((1,), {}, b'O|$OO', ['', '', 'a'])
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,7 +1089,7 @@ def test_attributes(self):
self.assertEqual(exc.name, 'somename')
self.assertEqual(exc.path, 'somepath')

msg = "'invalid' is an invalid keyword argument for this function"
msg = "'invalid' is an invalid keyword argument for ImportError"
with self.assertRaisesRegex(TypeError, msg):
ImportError('test', invalid='keyword')

Expand Down
13 changes: 7 additions & 6 deletions Lib/test/test_getargs2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,8 @@ def test_required_args(self):
try:
getargs_keywords(arg1=(1,2))
except TypeError as err:
self.assertEqual(str(err), "Required argument 'arg2' (pos 2) not found")
self.assertEqual(
str(err), "function missing required argument 'arg2' (pos 2)")
else:
self.fail('TypeError should have been raised')

Expand Down Expand Up @@ -626,16 +627,16 @@ def test_required_args(self):
)
# required arg missing
with self.assertRaisesRegex(TypeError,
r"Required argument 'required' \(pos 1\) not found"):
r"function missing required argument 'required' \(pos 1\)"):
getargs_keyword_only(optional=2)

with self.assertRaisesRegex(TypeError,
r"Required argument 'required' \(pos 1\) not found"):
r"function missing required argument 'required' \(pos 1\)"):
getargs_keyword_only(keyword_only=3)

def test_too_many_args(self):
with self.assertRaisesRegex(TypeError,
r"Function takes at most 2 positional arguments \(3 given\)"):
r"function takes at most 2 positional arguments \(3 given\)"):
getargs_keyword_only(1, 2, 3)

with self.assertRaisesRegex(TypeError,
Expand Down Expand Up @@ -674,11 +675,11 @@ def test_required_args(self):
self.assertEqual(self.getargs(1), (1, -1, -1))
# required positional arg missing
with self.assertRaisesRegex(TypeError,
r"Function takes at least 1 positional arguments \(0 given\)"):
r"function takes at least 1 positional arguments \(0 given\)"):
self.getargs()

with self.assertRaisesRegex(TypeError,
r"Function takes at least 1 positional arguments \(0 given\)"):
r"function takes at least 1 positional arguments \(0 given\)"):
self.getargs(keyword=3)

def test_empty_keyword(self):
Expand Down
64 changes: 42 additions & 22 deletions Python/getargs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1584,7 +1584,7 @@ PyArg_ValidateKeywordArguments(PyObject *kwargs)
}
if (!_PyDict_HasOnlyStringKeys(kwargs)) {
PyErr_SetString(PyExc_TypeError,
"keyword arguments must be strings");
"keywords must be strings");
return 0;
}
return 1;
Expand Down Expand Up @@ -1655,7 +1655,7 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
nkwargs = (kwargs == NULL) ? 0 : PyDict_GET_SIZE(kwargs);
if (nargs + nkwargs > len) {
PyErr_Format(PyExc_TypeError,
"%s%s takes at most %d argument%s (%zd given)",
"%.200s%s takes at most %d argument%s (%zd given)",
(fname == NULL) ? "function" : fname,
(fname == NULL) ? "" : "()",
len,
Expand Down Expand Up @@ -1705,8 +1705,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
}
if (max < nargs) {
PyErr_Format(PyExc_TypeError,
"Function takes %s %d positional arguments"
"%.200s%s takes %s %d positional arguments"
" (%d given)",
(fname == NULL) ? "function" : fname,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you changed from uppercase Function to lowercase “function”?

It may be worth changing an exception message if you can make it less confusing or easier to read. But changing just the letter case does not seem worth the pain of breaking the old message.

Copy link
Member

Choose a reason for hiding this comment

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

I suggested this for uniformity with other error messages.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I didn’t notice, and it’s not really a big deal. It just seemed a style preference thing to me.

(fname == NULL) ? "" : "()",
(min != INT_MAX) ? "at most" : "exactly",
max, nargs);
return cleanreturn(0, &freelist);
Expand Down Expand Up @@ -1752,8 +1754,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
* or the end of the format. */
}
else {
PyErr_Format(PyExc_TypeError, "Required argument "
"'%s' (pos %d) not found",
PyErr_Format(PyExc_TypeError, "%.200s%s missing required "
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't be better to omit the word "function" if the function name is not specified? Just "missing required argument 'foo' (pos 2)". What are your thoughts? Is it correct English and does it look better?

Copy link
Contributor Author

@MSeifert04 MSeifert04 Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I know that almost all other error messages used "function" or "this function" as placeholder when there's no name. But, yes, I mostly included it for consistency reasons, I have no preference either way.

(Note: I'm non native speaker so I would have to guess about correctness and/or looks)

Copy link
Member

Choose a reason for hiding this comment

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

“Missing required argument ‘foo’ (pos 2)” is correct English.

Whether including “function” may help would depend on the context. It may help when it is not clear from the exception context that a function call is involved. On the other hand, omitting the word makes the message simpler when the context is already clear.

"argument '%s' (pos %d)",
(fname == NULL) ? "function" : fname,
(fname == NULL) ? "" : "()",
kwlist[i], i+1);
return cleanreturn(0, &freelist);
}
Expand All @@ -1779,8 +1783,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,

if (skip) {
PyErr_Format(PyExc_TypeError,
"Function takes %s %d positional arguments"
"%.200s%s takes %s %d positional arguments"
" (%d given)",
(fname == NULL) ? "function" : fname,
(fname == NULL) ? "" : "()",
(Py_MIN(pos, min) < i) ? "at least" : "exactly",
Py_MIN(pos, min), nargs);
return cleanreturn(0, &freelist);
Expand All @@ -1802,8 +1808,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
if (current_arg) {
/* arg present in tuple and in dict */
PyErr_Format(PyExc_TypeError,
"Argument given by name ('%s') "
"argument for %.200s%s given by name ('%s') "
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't be better to omit the words "for function" if the function name is not specified? Just "argument given by name ('foo') and position (2)". What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

“For function” probably doesn’t add much value to the exception messsage in this case. But it does not seem very important to remove it either: it is a judgement call.

"and position (%d)",
(fname == NULL) ? "function" : fname,
(fname == NULL) ? "" : "()",
kwlist[i], i+1);
return cleanreturn(0, &freelist);
}
Expand All @@ -1826,8 +1834,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
if (!match) {
PyErr_Format(PyExc_TypeError,
"'%U' is an invalid keyword "
"argument for this function",
key);
"argument for %.200s%s",
key,
(fname == NULL) ? "this function" : fname,
(fname == NULL) ? "" : "()");
return cleanreturn(0, &freelist);
}
}
Expand Down Expand Up @@ -2060,7 +2070,7 @@ vgetargskeywordsfast_impl(PyObject **args, Py_ssize_t nargs,
}
if (nargs + nkwargs > len) {
PyErr_Format(PyExc_TypeError,
"%s%s takes at most %d argument%s (%zd given)",
"%.200s%s takes at most %d argument%s (%zd given)",
(parser->fname == NULL) ? "function" : parser->fname,
(parser->fname == NULL) ? "" : "()",
len,
Expand All @@ -2070,7 +2080,9 @@ vgetargskeywordsfast_impl(PyObject **args, Py_ssize_t nargs,
}
if (parser->max < nargs) {
PyErr_Format(PyExc_TypeError,
"Function takes %s %d positional arguments (%d given)",
"%200s%s takes %s %d positional arguments (%d given)",
(parser->fname == NULL) ? "function" : parser->fname,
(parser->fname == NULL) ? "" : "()",
(parser->min != INT_MAX) ? "at most" : "exactly",
parser->max, nargs);
return cleanreturn(0, &freelist);
Expand Down Expand Up @@ -2115,15 +2127,19 @@ vgetargskeywordsfast_impl(PyObject **args, Py_ssize_t nargs,
if (i < pos) {
Py_ssize_t min = Py_MIN(pos, parser->min);
PyErr_Format(PyExc_TypeError,
"Function takes %s %d positional arguments"
"%.200s%s takes %s %d positional arguments"
" (%d given)",
(parser->fname == NULL) ? "function" : parser->fname,
(parser->fname == NULL) ? "" : "()",
min < parser->max ? "at least" : "exactly",
min, nargs);
}
else {
keyword = PyTuple_GET_ITEM(kwtuple, i - pos);
PyErr_Format(PyExc_TypeError, "Required argument "
"'%U' (pos %d) not found",
PyErr_Format(PyExc_TypeError, "%.200s%s missing required "
"argument '%U' (pos %d)",
(parser->fname == NULL) ? "function" : parser->fname,
(parser->fname == NULL) ? "" : "()",
keyword, i+1);
}
return cleanreturn(0, &freelist);
Expand Down Expand Up @@ -2153,8 +2169,10 @@ vgetargskeywordsfast_impl(PyObject **args, Py_ssize_t nargs,
if (current_arg) {
/* arg present in tuple and in dict */
PyErr_Format(PyExc_TypeError,
"Argument given by name ('%U') "
"argument for %.200s%s given by name ('%U') "
"and position (%d)",
(parser->fname == NULL) ? "function" : parser->fname,
(parser->fname == NULL) ? "" : "()",
keyword, i+1);
return cleanreturn(0, &freelist);
}
Expand Down Expand Up @@ -2184,8 +2202,10 @@ vgetargskeywordsfast_impl(PyObject **args, Py_ssize_t nargs,
if (!match) {
PyErr_Format(PyExc_TypeError,
"'%U' is an invalid keyword "
"argument for this function",
keyword);
"argument for %.200s%s",
keyword,
(parser->fname == NULL) ? "this function" : parser->fname,
(parser->fname == NULL) ? "" : "()");
}
return cleanreturn(0, &freelist);
}
Expand Down Expand Up @@ -2365,7 +2385,7 @@ unpack_stack(PyObject **args, Py_ssize_t nargs, const char *name,
if (name != NULL)
PyErr_Format(
PyExc_TypeError,
"%s expected %s%zd arguments, got %zd",
"%.200s expected %s%zd arguments, got %zd",
name, (min == max ? "" : "at least "), min, nargs);
else
PyErr_Format(
Expand All @@ -2384,7 +2404,7 @@ unpack_stack(PyObject **args, Py_ssize_t nargs, const char *name,
if (name != NULL)
PyErr_Format(
PyExc_TypeError,
"%s expected %s%zd arguments, got %zd",
"%.200s expected %s%zd arguments, got %zd",
name, (min == max ? "" : "at most "), max, nargs);
else
PyErr_Format(
Expand Down Expand Up @@ -2469,7 +2489,7 @@ _PyArg_NoKeywords(const char *funcname, PyObject *kwargs)
return 1;
}

PyErr_Format(PyExc_TypeError, "%s does not take keyword arguments",
PyErr_Format(PyExc_TypeError, "%.200s does not take keyword arguments",
funcname);
return 0;
}
Expand All @@ -2486,7 +2506,7 @@ _PyArg_NoStackKeywords(const char *funcname, PyObject *kwnames)
return 1;
}

PyErr_Format(PyExc_TypeError, "%s does not take keyword arguments",
PyErr_Format(PyExc_TypeError, "%.200s does not take keyword arguments",
funcname);
return 0;
}
Expand All @@ -2504,7 +2524,7 @@ _PyArg_NoPositional(const char *funcname, PyObject *args)
if (PyTuple_GET_SIZE(args) == 0)
return 1;

PyErr_Format(PyExc_TypeError, "%s does not take positional arguments",
PyErr_Format(PyExc_TypeError, "%.200s does not take positional arguments",
funcname);
return 0;
}
Expand Down