Skip to content

fix union default - no need for builtin dynamic default#345

Closed
rmorshea wants to merge 2 commits intoipython:masterfrom
rmorshea:fix_union_default
Closed

fix union default - no need for builtin dynamic default#345
rmorshea wants to merge 2 commits intoipython:masterfrom
rmorshea:fix_union_default

Conversation

@rmorshea
Copy link
Contributor

@rmorshea rmorshea commented Nov 10, 2016

Closes #344

No more need for dynamic default in Union as a result of #332.

Also fixes a test case that was inheriting from HasTraits instead of TestCase, which masked #344.

@rmorshea rmorshea changed the title fix union default - trigger subclass init fix union default - no need for builtin dynamic default Nov 10, 2016
@jakevdp
Copy link
Contributor

jakevdp commented Nov 10, 2016

Awesome – thanks!

@SylvainCorlay
Copy link
Member

@rmorshea the reversion of the trait types validation is a backward incompatible change (we rely on the current order in bqplot).

What is the reason for the inversion?

@rmorshea
Copy link
Contributor Author

rmorshea commented Nov 10, 2016

@SylvainCorlay, since validation prefers the first given TraitType, it seemed reasonable to prefer the default value of the first given TraitType as well.

I'll put that in a separate PR though since I didn't think about compatibility when including that here.

ignore deleted comments - I second guessed myself

@jakevdp
Copy link
Contributor

jakevdp commented Nov 11, 2016

If validation order is an important part of the API, it might be worth putting in a unit test that will catch if that ever breaks.

@SylvainCorlay
Copy link
Member

Ah now I understand. Sorry I was mistaken. I agree that it would be more logical for it to be reversed.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Nov 11, 2016

If validation order is an important part of the API, it might be worth putting in a unit test that will catch if that ever breaks.

It is just that the first trait type that validate is the type that will be used.

So if a given value works for two of the types, it might be validated as one or the other depending of the order that is used.

Then, in the case of e.g. widgets, the serialization might be different for these two types.

@rmorshea
Copy link
Contributor Author

it might be worth putting in a unit test that will catch if [union validation order] ever breaks.

#347

@rmorshea rmorshea closed this Nov 13, 2016
@rmorshea
Copy link
Contributor Author

Found a more fundamental bug. Will rework this shortly.

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.

Default value of Union is not respected

3 participants