-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the use of cTrait.default_value to set the default value #1632
Remove the use of cTrait.default_value to set the default value #1632
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM with a couple of comments/questions.
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
…ated-default-value-for-set
@rahulporuri Thanks for the review! I've updated. If you have bandwidth for another look, that would be much appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a question
+----------------------------------------------------------------------------*/ | ||
|
||
static PyObject * | ||
_trait_default_value(trait_object *trait, PyObject *args) | ||
_trait_default_value(trait_object *trait, PyObject *Py_UNUSED(ignored)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to define a second argument to the _trait_default_value
function, which is then marked as ignored (IIUC this is done only to silence compiler warnings)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a second argument is required here. You can see lots of uses of this pattern in the CPython source, too. The function has to be of type PyCFunction
(docs here), which is defined as
typedef PyObject *(*PyCFunction)(PyObject *, PyObject *);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and yes, the Py_UNUSED
is there to silence compiler warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference and explanation. Still LGTM
The use of
cTrait.default_value
to set the default value type and default value was deprecated in 2019, in #620. This PR removes that deprecated support. To set the default value type and default value for aCTrait
orcTrait
instance, useset_default_value
.Checklist
docs/source/traits_api_reference
) N/Adocs/source/traits_user_manual
) N/Atraits-stubs
N/A