Skip to content

Fix copy(HasTraits) by copying _trait_values dict#482

Merged
minrk merged 4 commits intoipython:masterfrom
ankostis:fix_copy
Apr 2, 2018
Merged

Fix copy(HasTraits) by copying _trait_values dict#482
minrk merged 4 commits intoipython:masterfrom
ankostis:fix_copy

Conversation

@ankostis
Copy link
Contributor

@ankostis
Copy link
Contributor Author

All test-cases pass, except from Appveyor on Windows (expecting #483 to merge for that).

@minrk
Copy link
Member

minrk commented Mar 22, 2018

Makes sense. I'm not sure what to do when cross_validation_lock is held. It does make sense to clear it, but what's the state going to be if a HasTraits is copied in the middle of pending changes? Should the cross-validation be forced to finish as part of the copy?

ankostis added a commit to ankostis/traitlets that referenced this pull request Mar 22, 2018
- As agreed by
rmorshea(ipython#481 (comment))
  & minrk(ipython#482 (comment)).
- Added FIXME remark about pending resolution of what to do in case the
lock is held.
@ankostis
Copy link
Contributor Author

Ok, pinned the _cross_validation_lock=False when cloning.

Should the cross-validation be forced to finish as part of the copy?

Maybe raise an exception, to fail early?

@ankostis
Copy link
Contributor Author

Btw, I have pushed a relevant commit about utils.sentinel.Sentinel ignoring copy/deepcopy.
Sentinels must not change, or cloned traits fail defaults
(a a really nasty bug discovered a year ago, please don't ask me more).

@minrk
Copy link
Member

minrk commented Apr 2, 2018

Thanks!

@minrk minrk merged commit b775646 into ipython:master Apr 2, 2018
@Carreau Carreau added this to the 5.0 milestone Jun 4, 2020
@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc) and removed 5.0-re-review Need to re-review for potential API impact changes. labels Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants