gh-130821: Add type information to wrong type error messages - #130835
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?
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>
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
When an item yielded by a class namespace's items() is not iterable during abstract-method computation, include the type of the offending item in the raised TypeError, matching the type-information convention established in pythongh-130835. This targets the same _abc.c hunk that pythongh-144737 (now stale) proposed, using PyErr_ExceptionMatches to only replace the message on a genuine TypeError instead of passing NULL through PySequence_Fast, which risks a NULL PyErr_SetString call if that path is ever reached with a different underlying error.
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?