change CastError to describe the isinstance failure#5693
change CastError to describe the isinstance failure#5693davidhewitt merged 4 commits intoPyO3:mainfrom
CastError to describe the isinstance failure#5693Conversation
LilyFirefly
left a comment
LilyFirefly
left a comment
There was a problem hiding this comment.
The main change here looks good to me. I did spot a confusing test, but tidying that up isn't a blocker.
| assert_eq!( | ||
| err.to_string(), | ||
| "'NoneType' object cannot be cast as 'int | NoneType'" | ||
| "'NoneType' object is not an instance of 'int | NoneType'" |
There was a problem hiding this comment.
This test is weird. Is it important that we have NoneType twice, or would it be clearer to use, say, int | str? Because NoneType object (i.e. None) not being an instance of NoneType is at best very confusing, if not outright wrong.
There was a problem hiding this comment.
Hmm, good point. I wonder if in general we should consider special casing None in the error message too? e.g. 'None' is not an instance of instead of 'NoneType' object is not an instance of
There was a problem hiding this comment.
That sounds nice to have.
|
Thanks all for the approvals! I updated this to have the special cases for |
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
Happy new year! No concerns from my side.
4 participants
As discussed in #5472 (comment), this improves the clarity of the failure in
.cast()operations.cc @Icxolu @orlp