Resolve pickling and deepcopying bug with dynamically added traits #373
Conversation
* upstream/master: (907 commits) MAINT: Remove unused file BUG: Fix validation of This traits. Return None rather than raising. This is what would have happened if we'd been initializing all the CTrait fields properly in the first place. Don't segfault when asked for the default value of a trait that doesn't have one. Update Python versions supported in docs. Restore Appveyor support for 2.7 and 3.4. Update appveyor to use run_with_env.cmd from https://github.com/ogrisel/python-appveyor-demo Experimental commit to see if we can get Appveyor support working for 3.5 and 3.6. Fix a misuse of the unittest.expectedFailure decorator. Add Python 3.5 and Python 3.6 for Appveyor builds. Add Python 3.6 to Travis matrix. Raise for bad Python versions in the setup script; add relevant Trove classifiers. Update travis-ci-requirements. Drop Python 3.3 from appveyor matrix. Drop 2.6 and 3.3 from Travis CI matrix. Document Python version requirements. Version number bump for 4.7 development. Fix bad classifier. Release 4.6.0! Minor changes to release statement. ...
traits/has_traits.py
Outdated
| self.trait_set( trait_change_notify = trait_change_notify, **state ) | ||
| self._post_init_trait_listeners() | ||
| self.traits_init() | ||
| for attr, trait in inst_traits.iteritems(): |
There was a problem hiding this comment.
@cfarrow I notice that here we're calling add_trait after doing traits_init() and _post_init_trait_listeners(), but in the clone_traits block below, we're calling add_trait before those calls. Do you know whether there's a reason for the difference?
There was a problem hiding this comment.
I don't know the reason. I'll look into the potential consequences. At the very least I'll make it consistent.
There was a problem hiding this comment.
According to the doc string, traits_init should be called at the end of __setstate__. In practice, I don't think it matters. It is a no-op by default. The only place I have found it defined in the traits source is in HasUniqueStrings.
In any case, I moved it after all the add_traits to be consistent with the existing documentation and the behavior in clone_traits.
There was a problem hiding this comment.
Thanks, Chris. I also moved the _post_init_trait_listeners call, so that it's consistent with clone_traits.
traits/tests/test_regression.py
Outdated
| # All the counts should be the same. | ||
| self.assertEqual(counts[warmup:-1], counts[warmup+1:]) | ||
|
|
||
| def test_hastraits_pickle_deepcopy(self): |
There was a problem hiding this comment.
It looks as though this is a single test that tests both the pickling and the deepcopy behaviour in one. I think it should be split into two separate tests.
|
Thanks for the feedback. I've addressed the comments. |
|
Ah, python 3 test failures! I'm investigating. |
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
+ Coverage 64.03% 64.32% +0.29%
==========================================
Files 48 48
Lines 7373 7381 +8
Branches 1472 1474 +2
==========================================
+ Hits 4721 4748 +27
+ Misses 2212 2198 -14
+ Partials 440 435 -5
Continue to review full report at Codecov.
|
|
Everything is working now. |
|
Waiting for CI, then merging. |
|
Random failure at egg-fetching stage with Appveyor; restarting the job ... |
|
Merged. Thanks, @cfarrow! |
…-traits Revert #373: instance traits are no longer pickleable
4 participants
This is #23 with working tests.
Fixes #2
Fixes #16
Closes #23
The fix looks good to me. Any other takers?