msg176202 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-23 18:03 |
This came up in #16515.
While using PyArg_UnpackTuple to parse the positional arguments in a function that receives both positional and keyword arguments, the error message returned when the number of arguments is incorrect is misleading, e.g.:
>>> max(foo=1)
TypeError: max expected 1 arguments, got 0
This can be fixed by adding "positional" before "arguments" in the error message. The attached patch fixes this and the pluralization of "argument(s)".
|
msg176205 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2012-11-23 18:20 |
> + (min == 1 ? "" : "s"), l);
In second part of patch should be "max". Reformat the code so that `min` and `(min == 1 ? "" : "s")` will be in one line and it will be more clear.
|
msg176207 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-23 18:29 |
Now that I look at uses of PyArg_UnpackTuple, I'm wondering whether this needs to be fixed at all. Apart from `max` and `min`, do you know of any other cases where this gives a misleading error message?
Almost all the uses I can find are for simple functions/methods where all arguments are positional-only. (Ex: range, pow, slice, dict.pop).
max and min *do* clearly need an error message fix.
(Apologies: I know it was me who suggested that PyArg_UnpackTuple needed fixing in the first place. But now I'm not sure that's true.)
|
msg176208 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-23 18:30 |
Of course, the 'arguments' -> 'argument' fix would still be nice to have.
|
msg176209 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-23 18:36 |
> do you know of any other cases where this gives a misleading error message?
Actually my concern was about cases where the new error might be wrong/misleading, but I haven't checked yet. If there are no such cases I think we should fix it; the error message will be better in the few cases we have in the stdlib and for all the external libs that are using the function.
Another thing that I haven't checked is if there are similar functions (e.g. to parse kwargs) that needs the same treatment (#16520 might be one such example).
|
msg176210 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-23 18:37 |
One more thing: I'm not sure this is the right terminology. Here we're using 'positional' to refer to parameters that PEP 362 calls "POSITIONAL_ONLY", whereas the regular TypeErrors (e.g. produced for user-defined functions) use 'positional' to mean what PEP 362 calls "POSITIONAL_OR_KEYWORD". This seems like it'll only increase the current confusion between different parameter types.
|
msg176212 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-23 18:44 |
> If there are no such cases I think we should fix it.
Okay, but what exactly is this fixing? I can't find any examples apart from max and min where the new error messages seem any better than the old. Your first message talks about "a function that receives both positional and keyword arguments", by which I guess you mean 'both required and optional arguments'; in any case, I can't find any such function (apart from max and min) that uses PyArg_ParseTuple. So it seems to me that there's not actually any problem to be solved.
(And I think max and min probably need to be reworked to not use PyArg_ParseTuple at all.)
|
msg176213 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-23 18:47 |
> Okay, but what exactly is this fixing?
If the function parses positional args, it should say so in the error message. If the function is never used (except for min/max) or if the error is never reported, then we have a different problem.
Even if this code is not used I don't think we can remove it, since this it's a public function, and even if we are not using it someone might be using it and benefit from the improved error message.
|
msg176218 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-11-23 19:20 |
> Here we're using 'positional' to refer to parameters that PEP 362 calls "POSITIONAL_ONLY", whereas the regular TypeErrors (e.g. produced for user-defined functions) use 'positional' to mean what PEP 362 calls "POSITIONAL_OR_KEYWORD".
Here the error message is about "positional arguments," which are different from parameters. (As you know, issue 15990 is to document this distinction in the glossary.)
Incidentally, it looks like the documentation for several of the functions in this part of the C API should probably be updated in this regard, for example:
http://docs.python.org/dev/c-api/arg.html#PyArg_UnpackTuple
http://docs.python.org/dev/c-api/arg.html#PyArg_ParseTuple
The PyArg_UnpackTuple documentation says, for example:
"A simpler form of parameter retrieval which does not use a format string to specify the types of the arguments. Functions which use this method to retrieve their parameters..."
The two occurrences of "parameter" here should be changed to "argument."
|
msg176223 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-11-23 19:44 |
> This can be fixed by adding "positional" before "arguments" in the error message.
Do we know for sure that the values in the args argument in a call to PyArg_UnpackTuple always correspond to the positional arguments of a call to a Python function? Can PyArg_UnpackTuple be used in other ways?
|
msg176226 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-23 19:53 |
> Here the error message is about "positional arguments," which are
> different from parameters.
Okay. So is it also planned to change the existing error messages for user-defined functions? E.g.:
>>> def f(a, b=1):
... pass
...
>>> f()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: f() missing 1 required positional argument: 'a'
If yes, then I think all these changes need to be coordinated, and that probably involves submitting a proposal to the mailing list. If no, then I think the change proposed in this issue is not an improvement, since it makes the error messages overall inconsistent.
|
msg176229 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-23 20:00 |
Maybe we should follow a more systematic approach and collect the different errors raised in the different situations, and then come up with error messages that are accurate and follow the terminology that we are adopting in #15990.
|
msg176230 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-11-23 20:02 |
> TypeError: f() missing 1 required positional argument: 'a'
Yes, I think this should also be changed because passing "a" as a keyword argument is okay:
>>> f(a=1)
I would suggest something like--
TypeError: f() missing argument for parameter 'a'
|
msg176232 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-23 20:06 |
I would be fine with just dropping "positional" there, and say "required argument". With no further specification it means the "default" type of argument, i.e. positional or keyword. See also the end of msg170876.
|
msg176234 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-11-23 20:08 |
> TypeError: f() missing 1 required positional argument: 'a'
By the way, changing this message is the subject of issue 16520 (which should probably be retitled).
|
msg176236 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-23 20:11 |
#16520 could be assimilated by this issue, if we decide to widen its scope as I suggested in msg176229.
|
msg176237 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-11-23 20:11 |
+1 to msg176229
|
msg176241 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-11-23 20:23 |
I would keep any "global" discussion (for decision and coordination purposes) as part of a single meta-issue, but retain individual instances that need to be corrected as separate issues. I don't think we should try to fix them all as part of one patch or issue. I'm okay with the discussion happening here, or with opening a fresh issue that points to the various issues (including this one).
|
msg176243 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-23 20:27 |
Using this as meta-issue is fine -- there are already enough issues :)
The issues can probably be fixed in a single patch too, since it should just be a matter of changing the text of a few error messages.
|
msg176314 - (view) |
Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-11-24 20:12 |
> TypeError: f() missing 1 required positional argument: 'a'
The error message for round(), for example, has a different form:
>>> round()
TypeError: Required argument 'number' (pos 1) not found
It would be good if these had the same form.
|
msg176372 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-11-25 16:57 |
See also #15201.
|
msg176397 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2012-11-26 05:03 |
I think it’s Terry who has an enlightening distinction between arguments definition (positional-or-keyword, positional-only, keyword-only, starargs, starkwargs) and function call (positional, keyword, *-unpacked, **-unpacked). It may have been Nick or David though.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:38 | admin | set | github: 60747 |
2020-11-15 18:56:08 | iritkatriel | set | versions:
+ Python 3.9, Python 3.10, - Python 2.7, Python 3.2, Python 3.3, Python 3.4 |
2012-11-26 05:03:21 | eric.araujo | set | nosy:
+ terry.reedy, eric.araujo messages:
+ msg176397
|
2012-11-25 16:57:14 | ezio.melotti | set | messages:
+ msg176372 |
2012-11-24 20:25:50 | chris.jerdonek | link | issue16520 dependencies |
2012-11-24 20:23:20 | chris.jerdonek | set | assignee: ezio.melotti -> title: Use "positional arguments" in PyArg_UnpackTuple -> improve TypeError messages for missing arguments (meta issue) stage: test needed -> |
2012-11-24 20:12:39 | chris.jerdonek | set | messages:
+ msg176314 |
2012-11-23 20:27:23 | ezio.melotti | set | messages:
+ msg176243 |
2012-11-23 20:23:57 | chris.jerdonek | set | messages:
+ msg176241 |
2012-11-23 20:11:59 | mark.dickinson | set | messages:
+ msg176237 |
2012-11-23 20:11:23 | ezio.melotti | set | messages:
+ msg176236 |
2012-11-23 20:08:38 | chris.jerdonek | set | messages:
+ msg176234 |
2012-11-23 20:06:24 | ezio.melotti | set | messages:
+ msg176232 |
2012-11-23 20:02:57 | chris.jerdonek | set | messages:
+ msg176230 |
2012-11-23 20:00:50 | ezio.melotti | set | messages:
+ msg176229 |
2012-11-23 19:53:42 | mark.dickinson | set | messages:
+ msg176226 |
2012-11-23 19:44:38 | chris.jerdonek | set | messages:
+ msg176223 |
2012-11-23 19:20:32 | chris.jerdonek | set | messages:
+ msg176218 |
2012-11-23 18:50:27 | chris.jerdonek | set | nosy:
+ chris.jerdonek
|
2012-11-23 18:47:59 | ezio.melotti | set | messages:
+ msg176213 |
2012-11-23 18:44:47 | mark.dickinson | set | messages:
+ msg176212 |
2012-11-23 18:37:51 | mark.dickinson | set | messages:
+ msg176210 |
2012-11-23 18:36:06 | ezio.melotti | set | messages:
+ msg176209 |
2012-11-23 18:30:14 | mark.dickinson | set | messages:
+ msg176208 |
2012-11-23 18:29:40 | mark.dickinson | set | messages:
+ msg176207 |
2012-11-23 18:20:24 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg176205
|
2012-11-23 18:15:37 | mark.dickinson | set | nosy:
+ mark.dickinson
|
2012-11-23 18:07:53 | ezio.melotti | link | issue16515 dependencies |
2012-11-23 18:03:13 | ezio.melotti | create | |