Update catom to use fastcalls#232
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
=======================================
Coverage 97.67% 97.67%
=======================================
Files 24 24
Lines 1074 1074
Branches 162 162
=======================================
Hits 1049 1049
Misses 12 12
Partials 13 13 |
|
I think CAtom_setstate needs reworked, idk what I was doing back then... |
| if( PyTuple_GET_SIZE( args ) != 1 ) | ||
| return cppy::type_error( "__setstate__() takes exactly one argument" ); | ||
| PyObject* state = PyTuple_GET_ITEM( args, 0 ); | ||
| cppy::ptr itemsptr = PyMapping_Items(state); |
There was a problem hiding this comment.
Do we need a PyMapping_Check before this?
That said, PyMapping_Check doesn't even check for items().
Should we force it to be a dict?
Some of the below logic is questionable and may need some attention. I don't think that __setstate__ should trigger observers, which means it shouldn't go thru the __setattr__ protocol.
sccolbert
left a comment
sccolbert
left a comment
There was a problem hiding this comment.
I agree with everything except the __setstate__ method. I think that should be handled in a separate PR where we fix/agree on the behavior of that method.
frmdstryr
force-pushed
the
catom-fastcalls
branch
from
5cb71b2 to
d12ce27
Compare
|
Agree, I amended it to remove those changes. |
3 participants
No description provided.