Add allow_none flag for Callable trait #885
Conversation
|
I'm not sure whether |
It probably does: the validation in |
Codecov Report
@@ Coverage Diff @@
## master #885 +/- ##
==========================================
+ Coverage 72.58% 72.82% +0.24%
==========================================
Files 51 51
Lines 6471 6474 +3
Branches 1302 1302
==========================================
+ Hits 4697 4715 +18
+ Misses 1378 1360 -18
- Partials 396 399 +3
Continue to review full report at Codecov.
|
traits/ctraits.c
Outdated
|
|
||
| if ((value == Py_None) || PyCallable_Check(value)) { | ||
| if ((allow_none && value == Py_None) || PyCallable_Check(value)) { |
There was a problem hiding this comment.
This is something of a micro-optimisation, but I think we should only unpack and interpret the second element of trait->py_validate after we've checked value == Py_None: the == Py_None check is cheap, but the PyObject_IsTrue call will be more time-consuming.
We also need to catch the case where PyObject_IsTrue returns -1: in that case, something unexpected has happened and we should be returning NULL immediately.
traits/ctraits.c
Outdated
| goto done; | ||
| { | ||
| int allow_none = PyObject_IsTrue(PyTuple_GET_ITEM(type_info, 1)); | ||
| if ((allow_none && value == Py_None) || PyCallable_Check(value)) { |
There was a problem hiding this comment.
Similar comments to those for validate_trait_callable apply here: we need to do an error check on the return value of PyObject_IsTrue, and it probably makes sense to defer the PyObject_IsTrue call until after checking that value == Py_None.
traits/ctraits.c
Outdated
| @@ -4269,7 +4273,7 @@ _trait_set_validate(trait_object *trait, PyObject *args) | |||
| break; | |||
|
|
|||
| case 22: /* Callable check: */ | |||
| if (n == 1) { | |||
| if (n == 2) { | |||
There was a problem hiding this comment.
I'm wondering whether there's a potential pickle compatibility problem here: if a Callable trait has been pickled with Traits 6.0.0, does this check prevent it from being unpickled?
There was a problem hiding this comment.
That will be a problem since the expected number of arguments is different, I currently don't ensure that there are 2 arguments before I index into the second one. I think i'll count the args and also change the above to n <=2 and then choose the correct flow accordingly.
Fixes #773
PR adds a new optional argument
allow_noneto theCallabletrait, which if set toFalsepreventsNonefrom being assigned to the trait.If not specified, defaults to
allow_none=True, leaving the current behavior unchanged.