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

Fix validation of dynamic Enum inside a Tuple #1388

Merged
merged 6 commits into from
Jan 13, 2021
Merged

Conversation

mdickinson
Copy link
Member

This PR fixes validation of dynamic enumerations inside a Tuple. Previously, for the dynamic case of an Enum, no validation was performed until trait-set time. This PR changes the logic so that the validate method (which the Tuple uses for its item validation) is no longer None.

This fixes the issue originally reported in #1385. There are other issues identified in the #1385 comments that still need to be fixed (or have new issues opened) before #1385 can be closed.

Checklist

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

@mdickinson mdickinson changed the title Fix/enum in tuple Fix validation of dynamic Enum inside a Tuple Jan 13, 2021
@@ -2074,10 +2075,16 @@ def _get(self, object, name, trait):
return value

def _set(self, object, name, value):
""" Sets the current value of a dynamic range trait.
""" Sets the current value of a dynamic enum trait.
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated drive-by fix in this line, fixing copypasta ("range" -> "enum")

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM

@mdickinson
Copy link
Member Author

Thanks @kitchoi for reviewing. I added the new test (for a dynamic Enum inside a List) back in; it was failing before because I messed up the trait names. (Possibly a reason to use HasStrictTraits instead of HasTraits even when writing tests.)

@kitchoi
Copy link
Contributor

kitchoi commented Jan 13, 2021

Thank you! Still LGTM. So this PR can close #1385 then?

@mdickinson
Copy link
Member Author

So this PR can close #1385 then?

Yes, I guess so. I've opened an issue for the Tuple exception-silencing (#1389). I need to open an issue, or create a fix, for the corresponding issue in BaseRange.

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