Skip to content

Conversation

@donbarbos
Copy link
Contributor

@donbarbosdonbarbos commented Mar 4, 2025

I got next output for use case from issue:

Foo.__int__() must return an int, not NoneType Foo.__float__() must return a float, not NoneType Foo.__bytes__() must return a bytes, not NoneType Foo.__complex__() must return a complex, not NoneType Foo.__bool__() must return a bool, not NoneType Foo.__str__() must return a str, not NoneType 

but we still have a lot of other places where wrong type errors have inconsistent messages. should they be updated too?

Copy link
Contributor

@jstasiakjstasiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by comment: does it make sense to have tests for these changes?

@donbarbos
Copy link
ContributorAuthor

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

@skirpichevskirpichev self-requested a review April 28, 2025 13:19
Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much reason to change this error message either.

Copy link
ContributorAuthor

@donbarbosdonbarbosApr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I strongly prefer the old message. Iterable is not a type, it's a category of types.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about new message: "%T.%U() must return an iterable, not %T"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ..." :)

@serhiy-storchakaserhiy-storchaka self-requested a review July 6, 2025 08:28
@skirpichevskirpichev removed their request for review August 13, 2025 05:17
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

@serhiy-storchakaserhiy-storchaka merged commit 968f6e5 into python:mainAug 14, 2025
48 checks passed
@donbarbos
Copy link
ContributorAuthor

Thank you everyone for review

Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@donbarbos@jstasiak@JelleZijlstra@skirpichev@serhiy-storchaka