traits icon indicating copy to clipboard operation
traits copied to clipboard

Regression: Dynamic defaults for dynamic Enums are now validated

Open mdickinson opened this issue 3 years ago • 4 comments

PR #1388 fixed validation for dynamic Enums inside a container (e.g., a List or Tuple).

An unanticipated side-effect of that change is that dynamic defaults for dynamic Enums are also now validated, where previously they weren't. (The same will be true for dynamic Range traits.) This broke some code in a Traits-using project.

Given the following code:

from traits.api import Enum, HasStrictTraits, List


class HasDynamicEnumTrait(HasStrictTraits):
    allowed_languages = List(["JavaScript", "PHP"])

    selected_language = Enum(values="allowed_languages")

    def _selected_language_default(self):
        return "Python"


print(HasDynamicEnumTrait().selected_language)

Running this code under Traits 6.1.1 gives:

(traits) mdickinson@mirzakhani canopy_data % python ~/Desktop/bug.py  
JavaScript

while running the code under Traits 6.2.0 gives:

(traits) mdickinson@mirzakhani canopy_data % python ~/Desktop/bug.py  
Traceback (most recent call last):
  File "/Users/mdickinson/Desktop/bug.py", line 13, in <module>
    print(HasDynamicEnumTrait().selected_language)
  File "/Users/mdickinson/.venvs/traits/lib/python3.9/site-packages/traits/trait_types.py", line 2101, in _get
    value = self.get_value(object, name, trait)
  File "/Users/mdickinson/.venvs/traits/lib/python3.9/site-packages/traits/trait_type.py", line 323, in get_value
    object.__dict__[cname] = value = trait.default_value_for(
  File "/Users/mdickinson/.venvs/traits/lib/python3.9/site-packages/traits/trait_types.py", line 2119, in _validate
    self.error(object, name, value)
  File "/Users/mdickinson/.venvs/traits/lib/python3.9/site-packages/traits/base_trait_handler.py", line 74, in error
    raise TraitError(
traits.trait_errors.TraitError: The 'selected_language' trait of a HasDynamicEnumTrait instance must be 'JavaScript' or 'PHP', but a value of 'Python' <class 'str'> was specified.

mdickinson avatar Feb 10 '21 17:02 mdickinson

On next steps: I consider the validation of those dynamic defaults to be a Good Thing, but if this change adversely affects more than a tiny handful of Traits-using projects then we should consider reverting and making a bugfix release.

mdickinson avatar Feb 10 '21 17:02 mdickinson

One other thing: if we decide to keep the behaviour change, it would make sense to add some tests for the validation of the dynamic default.

mdickinson avatar Feb 10 '21 17:02 mdickinson

Given that in 6.1.1 the following raised an error:

from traits.api import Enum, HasStrictTraits, List

class HasDynamicEnumTrait(HasStrictTraits):

    selected_language = Enum("JavaScript", "PHP")

    def _selected_language_default(self):
        return "Python"


print(HasDynamicEnumTrait().selected_language)

I'm generally OK with the behaviour above.

However, this is more than a bit weird:

from traits.api import Enum, HasStrictTraits, List


class HasDynamicEnumTrait(HasStrictTraits):
    selected_language = Enum(values='allowed_languages')

    allowed_languages = List()

    def _selected_language_default(self):
        return "Python"

    def _allowed_languages_default(self):
        return ["JavaScript", "PHP", "Python"]


x = HasDynamicEnumTrait()
print(x.selected_language)
x.allowed_languages = ["JavaScript", "PHP"]
print(x.selected_language)

y = HasDynamicEnumTrait()
y.allowed_languages = ["JavaScript", "PHP"]
print(y.selected_language)

Which gives:

Python
JavaScript
Traceback (most recent call last):
  File "/Users/cwebster/src/scratch/enum_defaults.py", line 23, in <module>
    print(y.selected_language)
  File "/Users/cwebster/.edm/envs/edm/lib/python3.6/site-packages/traits/trait_types.py", line 2101, in _get
    value = self.get_value(object, name, trait)
  File "/Users/cwebster/.edm/envs/edm/lib/python3.6/site-packages/traits/trait_type.py", line 324, in get_value
    object, name
  File "/Users/cwebster/.edm/envs/edm/lib/python3.6/site-packages/traits/trait_types.py", line 2119, in _validate
    self.error(object, name, value)
  File "/Users/cwebster/.edm/envs/edm/lib/python3.6/site-packages/traits/base_trait_handler.py", line 75, in error
    object, name, self.full_info(object, name, value), value
traits.trait_errors.TraitError: The 'selected_language' trait of a HasDynamicEnumTrait instance must be 'JavaScript' or 'PHP', but a value of 'Python' <class 'str'> was specified.

Perhaps based on this an invalid default should instead return the first element of the enum?

corranwebster avatar Feb 11 '21 09:02 corranwebster

Argh. Yes, this definitely needs a rethink. The law of unintended consequences strikes again.

mdickinson avatar Feb 11 '21 10:02 mdickinson