Allow Collections in valid values for Enum trait#889
Conversation
Codecov Report
@@ Coverage Diff @@
## master #889 +/- ##
==========================================
+ Coverage 73.05% 73.24% +0.18%
==========================================
Files 51 51
Lines 6514 6521 +7
Branches 1309 1311 +2
==========================================
+ Hits 4759 4776 +17
+ Misses 1363 1352 -11
- Partials 392 393 +1
Continue to review full report at Codecov.
|
midhun-pm
changed the title
mdickinson
left a comment
mdickinson
left a comment
There was a problem hiding this comment.
I haven't reviewed the code in detail, but there are still a couple of things missing relative to the original issue:
- As described in the original issue, we should have documentation warning users about the arbitrary selection of a default when using something like a
setas a collection - Please include a test for the case
Enum(1, {1, 2}), whose behaviour changes as a result of this PR. - In addition to strings, we want to exclude bytestrings and bytearrays as collections, since
inbehaves oddly for those:
>>> from traits.api import *
>>> class A(HasTraits):
... foo = Enum(b"abc")
...
>>> a = A()
>>> a.foo
97
>>> a.foo = 98
>>> a.foo
98
>>> a.foo = 100 # Error message says 97, 98 and 99 are the only valid values
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/mdickinson/Enthought/ETS/traits/traits/base_trait_handler.py", line 76, in error
raise TraitError(
traits.trait_errors.TraitError: The 'foo' trait of an A instance must be 97 or 98 or 99, but a value of 100 <class 'int'> was specified.
>>> a.foo = b"bc" # But b"bc" is accepted as valid, too.
>>> a.foo
b'bc'
|
Please also update docstrings for |
mdickinson
left a comment
mdickinson
left a comment
There was a problem hiding this comment.
A few comments. I think there may be some opportunities to make the __init__ code a bit clearer and more consistent.
|
@midhun-pm When you've finished making changes, please could you click the "Re-request review" button? That way I'll be alerted that the PR is ready for re-review. |
There was a problem hiding this comment.
We're getting there. Please update enum_default: currently it accepts either a collection or a single string (or bytes or bytestring) object, which then gets treated as a single-element collection. That's a cruel and unusual API, and it also doesn't match the enum_default docstring. This is a somewhat general purpose utility function, and we should have a clean API for it.
I think it should be easy to rework the caller of enum_default so that it's always the collection that's being passed.
mdickinson
left a comment
mdickinson
left a comment
There was a problem hiding this comment.
A few more comments: it looks as though we may be able to get rid of is_collection entirely.
As part of this PR, there should be an update to the user documentation warning about the perils of using an unordered structure like set as the collections type.
| default_value = args[0] | ||
| self.values = tuple(args) | ||
|
|
||
| if isinstance(args, enum.EnumMeta): |
There was a problem hiding this comment.
@corranwebster Do you know what this branch is for? As far as I can tell, args will always be a tuple here, so this branch is never exercised, and can be removed. (Possibly it ended up here as a result of a bad merge?)
…hub.com:enthought/traits into feature/allow_collections_for_enum_valid_values
mdickinson
left a comment
mdickinson
left a comment
There was a problem hiding this comment.
Looking good; I think this is the last round of changes.
mdickinson
left a comment
mdickinson
left a comment
There was a problem hiding this comment.
Thanks for the updates! Merging.
3 participants
Fixes #835
PR modifies the
Enumtrait to allow values that are of typeCollection