msg385349 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-01-20 15:40 |
We currently say:
Traceback (most recent call last):
File "/Users/pgalindo3/github/python/master/lel.py", line 5, in <module>
A().foo(1,2)
TypeError: foo() takes 2 positional arguments but 3 were given
but we should say:
Traceback (most recent call last):
File "/Users/pgalindo3/github/python/master/lel.py", line 5, in <module>
A().foo(1,2)
TypeError: A.foo() takes 2 positional arguments but 3 were given. Did you forget 'self' in the method definition?
|
msg385354 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2021-01-20 17:13 |
I don't think this makes sense. How do you know there's a missing 'self'? Surely it's just as likely that there is indeed an extra argument. Especially if A.foo is defined by a library (stdib or 3rd party).
|
msg385358 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2021-01-20 18:26 |
Concur with Guido. Seems that that guess will be wrong in most cases.
|
msg385361 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-01-20 19:11 |
> I don't think this makes sense. How do you know there's a missing 'self'? Surely it's just as likely that there is indeed an extra argument. Especially if A.foo is defined by a library (stdib or 3rd party).
I use the same heuristic as Pypy: the callable is a bound method, the first name in the definition is not named "self" and it was called with n+1 positional arguments
|
msg385363 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-01-20 19:12 |
> Seems that that guess will be wrong in most cases.
Maybe I am missing something. Could you list some cases where this will give the wrong advice?
|
msg385371 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2021-01-20 20:55 |
> I use the same heuristic as Pypy: the callable is a bound method, the first name in the definition is not named "self" and it was called with n+1 positional arguments.
Okay, that makes sense, you could have said so in the bug or PR description. :-)
Do you also exclude class and static methods?
I still worry that this might confuse users when a library for some reason uses a different name than 'self'.
|
msg385372 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-01-20 21:00 |
> Do you also exclude class and static methods?
Yup, there is a test in the PR for that IIRC
>I still worry that this might confuse users when a library for some reason uses a different name than 'self'.
That's a good point but I think it should be fine because the old error is still there and the new part is formulated as a question (did you forgot ...?). We have other errors where we are not completely sure and we formulate the suggestion as a question. Some examples are the new error for incomplete imports and some missing coma errors.
|
msg385375 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2021-01-20 22:34 |
> Could you list some cases where this will give the wrong advice?
Guido already listed them. Library functions, static and class methods, and names like slf and _self (used either in local classes and wrapper functions to avoid conflict with the self parameter of the outer method or as a poor positional-only parameter).
> We have other errors where we are not completely sure and we formulate the suggestion as a question. Some examples are the new error for incomplete imports and some missing coma errors.
There were reasons for adding that suggestions. An error with partial recursive import is pretty common, but the error message did not have any clue about the prevalent cause of error.
The problem with missing comma in a sequence is that since the code is always multiline, the traceback never contains enough information about the problem. And we are sure that the only case when the suggestion is wrong is when you intentionally wrote incorrect code.
The problem with missing "self" does not look so common and hard to correctly determine as the above examples. We want to keep the interpreter so simple as possible, and the proposed feature looks to me far the bar of compromise.
|
msg385378 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-01-20 23:21 |
> Guido already listed them. Library functions, static and class methods,
PR 24272 will work on all those cases.
> and names like slf and _self (used either in local classes and wrapper functions to avoid conflict with the self parameter of the outer method or as a poor positional-only parameter).
That's fine, the error is a suggestion (as we do in other parts of the stdlib), and the places where "self" is not used is normally rare. The old error is still there. The cases you mention are valid but I would say there are the exception and if someone is writing them they will probably still be able to make sense of the error.
> The problem with missing "self" does not look so common and hard to correctly determine as the above examples.
Is true, but I think that the cases were this will raise a false positive is very rare and make the benefits of the error message appealing (at least to me).
> We want to keep the interpreter so simple as possible, and the proposed feature looks to me far the bar of compromise.
This is an excellent point. THe only reason I am giving this a go is because this error message is super confusing to newcomers and educators are asking us, again and again, to include this hint as pypy is doing currently.
|
msg385381 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-01-21 00:09 |
Of course, being said all this, if the general consensus is that is not worth the trouble, I am happy to retract the PR :)
|
msg385384 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2021-01-21 00:38 |
-0 for this PR. It would cause more problems than it solves, and traditionally we leave this sort of problem for linters and type checkers. Also, this kind of error surfaces readily with even minimal testing. So, I don't think this PR is worth it.
That said, don't stop looking for places where the parser can provide more useful error reporting. Some parsing errors are truly inscrutable.
|
msg385385 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-01-21 00:40 |
Thanks, everyone for your valuable comments and for explaining your concerns!
I will close the PR as it seems that there is no clear consensus that will be a win after balancing the problems.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:40 | admin | set | github: 87144 |
2021-01-21 00:45:55 | pablogsal | set | status: open -> closed resolution: rejected stage: patch review -> resolved |
2021-01-21 00:40:42 | pablogsal | set | messages:
+ msg385385 |
2021-01-21 00:38:48 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg385384
|
2021-01-21 00:09:55 | pablogsal | set | messages:
+ msg385381 |
2021-01-20 23:21:33 | pablogsal | set | messages:
+ msg385378 |
2021-01-20 22:34:51 | serhiy.storchaka | set | messages:
+ msg385375 |
2021-01-20 21:00:35 | pablogsal | set | messages:
+ msg385372 |
2021-01-20 20:55:07 | gvanrossum | set | messages:
+ msg385371 |
2021-01-20 19:12:58 | pablogsal | set | messages:
+ msg385363 |
2021-01-20 19:11:52 | pablogsal | set | messages:
+ msg385361 |
2021-01-20 18:26:40 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg385358
|
2021-01-20 17:13:10 | gvanrossum | set | nosy:
+ gvanrossum messages:
+ msg385354
|
2021-01-20 15:41:11 | pablogsal | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request23095 |
2021-01-20 15:40:27 | pablogsal | create | |