gh-130821: Add type information to wrong type error messages#130835
gh-130821: Add type information to wrong type error messages#130835serhiy-storchaka merged 17 commits intopython:mainfrom
Conversation
jstasiak
left a comment
jstasiak
left a comment
There was a problem hiding this comment.
Drive-by comment: does it make sense to have tests for these changes?
it seems that we don't check the text of error messages if we don't want to check the error type itself |
JelleZijlstra
left a comment
JelleZijlstra
left a comment
There was a problem hiding this comment.
I think many of the changes in wording are not clear improvements.
|
|
||
| if (!PyLong_Check(result)) { | ||
| PyErr_Format(PyExc_TypeError, | ||
| "__index__ returned non-int (type %.200s)", |
There was a problem hiding this comment.
I don't see much reason to change this error message either.
There was a problem hiding this comment.
I changed it to the more common way as you recommended elsewhere: __method__() must return an int, not ...
and added type info before method name, like for other methods
| PyThreadState *tstate = _PyThreadState_GET(); | ||
| if (_PyErr_ExceptionMatches(tstate, PyExc_TypeError)) { | ||
| _PyErr_Format(tstate, PyExc_TypeError, | ||
| "%.200s.%U() returned a non-iterable (type %.200s)", |
There was a problem hiding this comment.
Here I strongly prefer the old message. Iterable is not a type, it's a category of types.
There was a problem hiding this comment.
what do you think about new message: "%T.%U() must return an iterable, not %T"
There was a problem hiding this comment.
I don't think new message adds something bad. Why someone might think, that iterable is a type here? (Another similar case, where new messages mention an iterator.)
There was a problem hiding this comment.
Why someone might think, that iterable is a type here?
it's just that in the previous iteration the error message was the phrase "must return type ..." :)
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
Thank you everyone for review |
Update error messages in Modules/ directory to use consistent format with %T formatter for type names, similar to changes made in PR python#130835 for Objects/ directory. Changes: - Modules/_abc.c: items() error message - Modules/_datetimemodule.c: divmod() error message - Modules/_pickle.c: read() error message - Modules/_io/bufferedio.c: read() and readall() error messages - Modules/_io/iobase.c: read() error message - Modules/_io/textio.c: decoder and encoder error messages - Modules/_csv.c: iterator error message
… magic methods Add type information to error messages for __getnewargs__, __getnewargs_ex__, __hash__, and __init__ when they return the wrong type. This is a follow-up to pythonGH-130835, which fixed similar inconsistencies for other magic methods. Error messages now use the format 'Type.__method__() must return ...' consistently, replacing the old format that used '%.200s' for type names and lacked the type prefix. Changes: - __getnewargs_ex__: 'should return' -> '%T.__getnewargs_ex__() must return' - __getnewargs__: 'should return' -> '%T.__getnewargs__() must return' - __hash__: '__hash__ method should return an integer' -> '%T.__hash__() must return an integer, not %T' - __init__: 'should return None' -> 'must return None' with %T format
5 participants
I got next output for use case from issue:
but we still have a lot of other places where wrong type errors have inconsistent messages. should they be updated too?