classification
Title: PyArg_ParseTupleAndKeywords exception messages containing "function"
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: MSeifert, martin.panter, r.david.murray, serhiy.storchaka, vstinner
Priority: normal Keywords:

Created on 2017-03-31 00:03 by MSeifert, last changed 2017-06-09 16:27 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 916 merged MSeifert, 2017-03-31 00:04
PR 2028 merged serhiy.storchaka, 2017-06-09 13:58
Messages (12)
msg290885 - (view) Author: Michael Seifert (MSeifert) * Date: 2017-03-31 00:03
Some exceptions thrown by `PyArg_ParseTupleAndKeywords` refer to "function" or "this function" even when a function name was specified.

For example:

>>> import bisect
>>> bisect.bisect_right([1,2,3,4], 2, low=10)
TypeError: 'low' is an invalid keyword argument for this function

Wouldn't it be better to replace the "this function" part (if given) with the actual function name?
msg290894 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-31 05:45
LGTM in general. I was going to make similar changes.

While we a here, maybe add the function name to other TypeError messages specific to the function? "Required argument '%U' (pos %d) not found" and "Argument given by name ('%U') and position (%d)", but not to "keywords must be strings".

Classified this as an enhancement.
msg290896 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-31 06:10
And while we are here, please change "keywords must be strings" to "keyword arguments must be strings" as in PyArg_ValidateKeywordArguments(). And maybe make all TypeError messages starting from a lower letter for uniformity.
msg290902 - (view) Author: Michael Seifert (MSeifert) * Date: 2017-03-31 09:24
Thank you for the suggestions, I added them to the PR. If you want 

But are you sure about the "keywords must be strings" -> "keyword arguments must be strings" change? I always thought the key is the "keyword" and the value the "(keyword) argument".
msg290903 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-31 09:27
Martin, could you please take a look? Does the wording of error messages look good English? Do you have to suggest some enhancements?
msg290904 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-31 09:36
> But are you sure about the "keywords must be strings" -> "keyword arguments must be strings" change? I always thought the key is the "keyword" and the value the "(keyword) argument".

Now, after you have made change, I have doubts. Yes, it looks ambiguous. Maybe "keyword names"? Martin, David, what are your thoughts?
msg290907 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-03-31 12:02
In this test, “keyword arguments” is definitely wrong:

 >>> f(**{1:2})
-TypeError: f() keywords must be strings
+TypeError: f() keyword arguments must be strings

To me, a keyword argument is a _value_ passed in using the f(name=. . .) syntax, and the keyword is the name that identifies it (usually becomes a parameter name in the function implementation). So I think the original was better. But if you think the original can be confusing, “keyword names” would also work.

The other messages look okay to me.
msg290910 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-31 12:22
I agree. PyArg_ValidateKeywordArguments() fooled me. Now I see that it would be better to change only PyArg_ValidateKeywordArguments().
msg290931 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-03-31 17:29
If you want to be completely unambiguous, you could say "keyword argument names".  "keyword argument" appears to mean different things in different contexts; sometimes it means the name and the value together, sometimes one or the other.  Usually which one it is is clear from context, but it seems in this case it is not clear.
msg291356 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-09 07:47
New changeset 64c8f705c0121a4b45ca2c3bc7b47b282e9efcd8 by Serhiy Storchaka (Michael Seifert) in branch 'master':
bpo-29951: Include function name for some error messages in `PyArg_ParseTuple*` (#916)
https://github.com/python/cpython/commit/64c8f705c0121a4b45ca2c3bc7b47b282e9efcd8
msg291357 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-09 07:49
Thank you for your contribution Michael. Since most builtin and extension functions and methods now use Argument Clinic and provide a function name, this should be a great enhancement.
msg295553 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-09 16:27
New changeset f9f1ccace395a8f65b60dc12567a237b4002fd18 by Victor Stinner (Serhiy Storchaka) in branch 'master':
Fix regression in error message introduced in bpo-29951. (#2028)
https://github.com/python/cpython/commit/f9f1ccace395a8f65b60dc12567a237b4002fd18
History
Date User Action Args
2017-06-09 16:27:08vstinnersetnosy: + vstinner
messages: + msg295553
2017-06-09 13:58:04serhiy.storchakasetpull_requests: + pull_request2094
2017-04-09 07:49:51serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg291357

stage: patch review -> resolved
2017-04-09 07:47:15serhiy.storchakasetmessages: + msg291356
2017-03-31 17:29:06r.david.murraysetmessages: + msg290931
2017-03-31 12:22:46serhiy.storchakasetmessages: + msg290910
2017-03-31 12:02:46martin.pantersetmessages: + msg290907
2017-03-31 09:36:40serhiy.storchakasetnosy: + r.david.murray
messages: + msg290904
2017-03-31 09:27:11serhiy.storchakasetnosy: + martin.panter
messages: + msg290903
2017-03-31 09:24:26MSeifertsetmessages: + msg290902
2017-03-31 06:10:48serhiy.storchakasetmessages: + msg290896
2017-03-31 05:45:57serhiy.storchakasettype: behavior -> enhancement
components: + Interpreter Core
versions: - Python 3.5, Python 3.6
nosy: + serhiy.storchaka

messages: + msg290894
stage: patch review
2017-03-31 00:04:42MSeifertsetpull_requests: + pull_request816
2017-03-31 00:03:07MSeifertcreate