Fix TraitType clone default values #1645
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These should fail initially.
This intends to be deliberate about what needs to be done for different DefaultValue types. Some of the tests will still fail after this is added.
This is safe because the clone method on the trait type now correctly sets up default values.
mdickinson
reviewed
Jun 6, 2022
|
@corranwebster I think this is ready to go in. Any last thoughts before I merge? |
mdickinson
added
component: core
|
I'm good with the changes. |
Thanks! Merging. |
mdickinson
approved these changes
Aug 10, 2022
mdickinson
pushed a commit
that referenced
this pull request
Aug 10, 2022
This PR works towards a fix of #1630 and related issues. The starting point is a set of regression tests that identify the behaviour which doesn't seem correct and test for the expected behaviour. This then adds changes to TraitTypes.clone that fix a number of the observed issues. We then remove the default value wrangling in HasTraits as it should no longer be needed. I think at this point everything is in a better state than it was: we have less unexpected behaviour. There may be subsequent things which might be advisable, such as raising warnings about future deprecation of certain behaviours, adding clone methods to particular trait types, etc. Co-authored-by: Mark Dickinson <[email protected]> (cherry picked from commit 7ecd065)
mdickinson
removed
the
porting: needs backport to 6.4
mdickinson
added a commit
that referenced
this pull request
Aug 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft PR working towards a fix of #1630 and related issues.
The starting point is a set of regression tests that identify the behaviour which doesn't seem correct and test for the expected behaviour. This then adds changes to
TraitTypes.clonethat fix a number of the observed issues. We then remove the default value wrangling inHasTraitsas it should no longer be needed.I think at this point everything is in a better state than it was: we have less unexpected behaviour.
There may be subsequent things which might be advisable, such as raising warnings about future deprecation of certain behaviours, adding
clonemethods to particular trait types, etc.Checklist
docs/source/traits_api_reference)docs/source/traits_user_manual)Update type annotation hints intraits-stubs