Skip to content
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

Add test for the use case of Trait with mapping and callable #1336

Merged
merged 5 commits into from
Nov 13, 2020

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Nov 4, 2020

Motivated by #1258, this PR adds a few tests that exercise the scenario where migration from TraitMap to Map is hard.

The difficulty here is that Trait can take a number of things, and when a mapping and a callable are given to it, it makes the trait a mapped trait because there is a mapping. And when the callable is used for validation (and transformation), both the key value and the mapped value are the same. But when the callable fails, the mapping is used and the key value is not the same as the mapped value.

If we migrate to Map, then the key value would be different from the mapped value. If we use Union to combine them together, then the mapped trait is gone.

I still haven't figured out a migration solution for this scenario that does not involve just reimplement a TraitType that does all these things, but maybe that's what downstream projects have to do if they need to keep these strange behaviour alive, so that Traits can deprecate TraitMap and Trait.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-stubs

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; a couple of nitpicks.

traits/tests/test_traits.py Outdated Show resolved Hide resolved
traits/tests/test_traits.py Outdated Show resolved Hide resolved
Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
@kitchoi kitchoi merged commit 78551a8 into master Nov 13, 2020
@kitchoi kitchoi deleted the tst-unable-to-migrate-traitmap branch November 13, 2020 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants