enthought / traits

Observable typed attributes for Python classes
Other
436 stars 85 forks source link

Regression: Dynamic defaults for dynamic Enums are now validated #1434

Open mdickinson opened 3 years ago

mdickinson commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

corranwebster commented 3 years ago

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?

mdickinson commented 3 years ago

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