Restrict PrefixList signature to accept a list or tuple of values #1142
Conversation
traits/trait_types.py
Outdated
| self.values = values[:] | ||
| def __init__(self, values, **metadata): | ||
| if not isinstance(values, SequenceTypes): | ||
| raise ValueError( |
There was a problem hiding this comment.
I think this is a TypeError (wrong sort or number of parameters) rather than a ValueError (right sort or number of parameters, but bad values).
There was a problem hiding this comment.
Yep, it would be consistent with the test above!
There was a problem hiding this comment.
And the check was isinstance, so TypeError makes more sense indeed.
traits/trait_types.py
Outdated
| Either all legal string values for the enumeration, or a single list | ||
| or tuple of legal string values. | ||
| values | ||
| A single list or tuple of legal string values. |
There was a problem hiding this comment.
I'd be happy to modify this docstring not to advertise the tuple support; that support seems misplaced to me, and I'm not sure I want to promise such support going forward.
traits/trait_types.py
Outdated
| def __init__(self, values, **metadata): | ||
| if not isinstance(values, SequenceTypes): | ||
| raise TypeError( | ||
| "Legal values should be provided via a list, " |
There was a problem hiding this comment.
The mention of tuple is excluded here...making the error message inconsistent with the actual check.
There was a problem hiding this comment.
Maybe what we want to do is mimic what's in BaseEnum: do an explicit isinstance check to exclude str, bytes and bytearray, but otherwise do no checking and rely on duck typing.
There was a problem hiding this comment.
Now I am thinking we should just check isinstance(values, list).
There was a problem hiding this comment.
Race condition: I did not see @mdickinson's message when I posted mine. Duck-typing with the exclusion of strings and friends would do too.
There was a problem hiding this comment.
So looking at the code, I think we could replace self.values = values[:] in the __init__ with self.values = list(values) (after doing the check to exclude str and friends).
* Add changelog for 6.1, release summary is still missing * Comestic fixes * Move Misc item to the bottom * Add placeholder for release date; add recent PRs * Stash partial work before context switch * More updates; move #1058 to correct section; add #1127 * More updates; mostly formatting * Update CHANGES.rst Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com> * More tweaks to wording and content * More wording tweaks * Use short form for View reference * Make use of the intersphinx mapping * Rewordings * Remove broken links * Add recent PRs; fix some long lines * Combine duplicate change entries for Traits / TraitsUI compatibility * Add note about default versus default_value for Either and Union * Add back note that Either will eventually be deprecated. * Remove some duplicate entries; add some missing ones * Ordering * More PRs (sneakily adding #1142 before it's actually merged) Co-authored-by: Kit Yan Choi <kchoi@enthought.com> Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>
Closes #1137
This forces
PrefixListto haveat leastexactly one positional argument, and that legal values must be provided via a list or a tuple.Checklist
docs/source/traits_api_reference)docs/source/traits_user_manual)traits-stubs