Allow assigning None to CTrait.post_setattr #833

midhun-pm · 2020-01-27T16:07:21Z

Fix #827
Allow setting CTrait.post_setattr to None

codecov-io · 2020-01-27T16:15:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@22c9ecd). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #833   +/-   ##
=========================================
  Coverage          ?   72.45%           
=========================================
  Files             ?       51           
  Lines             ?     6367           
  Branches          ?     1278           
=========================================
  Hits              ?     4613           
  Misses            ?     1360           
  Partials          ?      394

Impacted Files	Coverage Δ
traits/api.py	`90.32% <ø> (ø)`
traits/trait_types.py	`69.73% <ø> (ø)`
traits/trait_base.py	`63.41% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22c9ecd...ce38f0a. Read the comment docs.

mdickinson

The code change looks good, but I think we could do with a better test here.

traits/tests/test_ctraits.py

traits/ctraits.c

mdickinson · 2020-01-30T09:03:07Z

traits/ctraits.c

@@ -4333,6 +4333,10 @@ post_setattr_trait_python(
    trait_object *trait, has_traits_object *obj, PyObject *name,
    PyObject *value)
 {
+    if (trait->py_post_setattr == NULL) {


Is this code branch ever exercised? If it is, that's a coding error, so we should be raising some sort of exception here. But I suspect it's not.

What happens if you add an assert(0) here, compile with -UNDEBUG, and then run the test suite?

This is not triggered if post_setattr is set to NULL whenever py_post_setattr is NULL.
As you suspected, the branch execution was a coding error caused by changes in this PR.

mdickinson · 2020-01-30T09:06:49Z

traits/ctraits.c

@@ -4841,9 +4845,13 @@ get_trait_post_setattr(trait_object *trait, void *closure)
 static int
 set_trait_post_setattr(trait_object *trait, PyObject *value, void *closure)
 {
-    if (!PyCallable_Check(value)) {
+    if(value == Py_None){


Style nit: add a space before the "(" and another after the ")".

mdickinson · 2020-01-30T09:11:19Z

traits/ctraits.c

        PyErr_SetString(
-            PyExc_ValueError, "The assigned value must be callable.");
+            PyExc_ValueError, "The assigned value must be callable or None.");


So I think we need to do more here: if we're setting None as a value for py_post_setattr, then having trait->post_setattr be post_setattr_trait_python is wrong. Instead, we want trait->post_setattr to be NULL as well.

The invariant should be that if trait->post_setattr is post_setattr_trait_python, then py_post_setattr is non-NULL.

mdickinson

We're getting there, but this still isn't quite right: we should ensure that when py_post_setattr is NULL, then post_setattr is NULL, too.

mdickinson · 2020-01-30T09:43:51Z

Stepping back a bit, here's the big picture:

We're interested in being able to get and set the post_setattr state from Python
Conceptually, there are two cases for that state:
- There's no post_setattr handler
- There's a Python callable object that serves as a post_setattr handler
  I believe that these are the only two cases. @midhun-pm is that your impression too?
  So in Python-land, we can represent the state by a "callable or None".
In ctraits.c, the state is represented by two fields: post_setattr and py_post_setattr, so we need to make sure that those two fields are updated together, and remain consistent with one another.

To map from the Python state to the C state, I think what we want is:

If there's no post_setattr handler for the given Trait, then both fields should be NULL. That way, the C code doesn't end up having to check the NULL-ness of both fields; it only needs to check one.
If there's a post_setattr handler, then the post_setattr field points to the special post_setattr_trait_python function, and py_post_setattr points to the Python-level callable.

@midhun-pm Does this make sense? I think you're probably more familiar with the C internal details than I am at this point.

It does look to me as though the trait internals actually only need one field for this, not two; the idea was presumably originally that there might be other C-level handlers for the post_setattr operation. But changing that is not a change we should make in this PR (and if we did make that change, care would be needed to make sure that we didn't break pickle compatibility).

midhun-pm · 2020-01-31T03:06:11Z

Thank you for the explanation ! I was confused by the presence of both fields. It is a lot more clearer to me now and have made the changes that you recommended.

mdickinson

LGTM. Thank you!

traits/ctraits.c

mdickinson

One tiny nitpick, and please revert the last commit.

traits/ctraits.c

This reverts commit c82895b.

Co-Authored-By: Mark Dickinson <mdickinson@enthought.com>

mdickinson

LGTM. Thanks for the updates!

Allow assigning None to CTrait.post_setattr

af928ec

mdickinson requested changes Jan 28, 2020

View reviewed changes

traits/tests/test_ctraits.py Show resolved Hide resolved

midhun-pm added 2 commits January 28, 2020 11:59

Fix a bug when calling py_post_setattr set to None

a427dff

Add a better unittest for testing None assignment to post_setattr

d80a82a

mdickinson reviewed Jan 29, 2020

View reviewed changes

traits/ctraits.c Outdated Show resolved Hide resolved

mdickinson reviewed Jan 29, 2020

View reviewed changes

traits/ctraits.c Show resolved Hide resolved

midhun-pm added 2 commits January 29, 2020 10:53

Convert Py_None to NULL before assigning to trait->py_post_setattr

22c9ecd

Merge branch 'master' into bug/allow_setting_ctrait_post_setattr_to_None

4abf867

mdickinson reviewed Jan 30, 2020

View reviewed changes

Keep py_post_setattr and post_setattr in sync

43ba7df

midhun-pm added 2 commits January 30, 2020 21:08

Style fix

f15ec3a

Style fix

874512c

mdickinson approved these changes Jan 31, 2020

View reviewed changes

DECREF Py_None when value is made none

c82895b

mdickinson reviewed Feb 3, 2020

View reviewed changes

traits/ctraits.c Outdated Show resolved Hide resolved

mdickinson requested changes Feb 3, 2020

View reviewed changes

mdickinson reviewed Feb 3, 2020

View reviewed changes

traits/ctraits.c Outdated Show resolved Hide resolved

midhun-pm and others added 2 commits February 3, 2020 09:24

Revert "DECREF Py_None when value is made none"

496aa30

This reverts commit c82895b.

Fix styling

ce38f0a

Co-Authored-By: Mark Dickinson <mdickinson@enthought.com>

mdickinson approved these changes Feb 3, 2020

View reviewed changes

mdickinson merged commit ad6f627 into master Feb 3, 2020

mdickinson deleted the bug/allow_setting_ctrait_post_setattr_to_None branch February 3, 2020 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow assigning None to CTrait.post_setattr #833

Allow assigning None to CTrait.post_setattr #833

midhun-pm commented Jan 27, 2020

codecov-io commented Jan 27, 2020 •

edited

mdickinson left a comment

mdickinson Jan 30, 2020

midhun-pm Jan 31, 2020

mdickinson Jan 30, 2020

mdickinson Jan 30, 2020

mdickinson left a comment

mdickinson commented Jan 30, 2020

midhun-pm commented Jan 31, 2020

mdickinson left a comment

mdickinson left a comment

mdickinson left a comment

Allow assigning None to CTrait.post_setattr #833

Allow assigning None to CTrait.post_setattr #833

Conversation

midhun-pm commented Jan 27, 2020

codecov-io commented Jan 27, 2020 • edited

Codecov Report

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson Jan 30, 2020

Choose a reason for hiding this comment

midhun-pm Jan 31, 2020

Choose a reason for hiding this comment

mdickinson Jan 30, 2020

Choose a reason for hiding this comment

mdickinson Jan 30, 2020

Choose a reason for hiding this comment

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson commented Jan 30, 2020

midhun-pm commented Jan 31, 2020

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 27, 2020 •

edited