gh-130821: Add type information to wrong type error messages - #130835
Conversation
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
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
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.
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?