From 0113ae4b5c573aff9c25e406ed2cc95c92ab63e6 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 12 May 2021 17:22:12 +0100 Subject: [PATCH 1/6] Fix that add_class_trait was broken for traits with items in the presence of subclasses. --- traits/has_traits.py | 18 +++++--- traits/tests/test_has_traits.py | 75 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/traits/has_traits.py b/traits/has_traits.py index 63dfb7734..2a3c0489c 100644 --- a/traits/has_traits.py +++ b/traits/has_traits.py @@ -1135,11 +1135,11 @@ def add_class_trait(cls, name, *trait): trait = trait_for(trait[0]) # Add the trait to the class: - cls._add_class_trait(name, trait, False) + cls._add_class_trait(name, trait, is_subclass=False) # Also add the trait to all subclasses of this class: for subclass in cls.trait_subclasses(True): - subclass._add_class_trait(name, trait, True) + subclass._add_class_trait(name, trait, is_subclass=True) @classmethod def _add_class_trait(cls, name, trait, is_subclass): @@ -1161,7 +1161,7 @@ def _add_class_trait(cls, name, trait, is_subclass): prefix_list.append(name) # Resort the list from longest to shortest: - prefix_list.sort(lambda x, y: len(y) - len(x)) + prefix_list.sort(key=lambda x: -len(x)) return @@ -1177,9 +1177,17 @@ def _add_class_trait(cls, name, trait, is_subclass): handler = trait.handler if handler is not None: if handler.has_items: - cls.add_class_trait(name + "_items", handler.items_event()) + cls._add_class_trait( + name + "_items", + handler.items_event(), + is_subclass=is_subclass, + ) if handler.is_mapped: - cls.add_class_trait(name + "_", mapped_trait_for(trait, name)) + cls._add_class_trait( + name + "_", + mapped_trait_for(trait, name), + is_subclass=is_subclass, + ) # Make the new trait inheritable (if allowed): if trait.is_base is not False: diff --git a/traits/tests/test_has_traits.py b/traits/tests/test_has_traits.py index 6bd00e915..edfa1eac6 100644 --- a/traits/tests/test_has_traits.py +++ b/traits/tests/test_has_traits.py @@ -464,6 +464,81 @@ class A(HasTraits): self.assertIsNot(objs_copy[0], objs[0]) self.assertIs(objs_copy[0], objs_copy[1]) + def test_add_class_trait(self): + # Testing basic usage. + class A(HasTraits): + pass + + A.add_class_trait("y", Str()) + + a = A() + + self.assertEqual(a.y, "") + + def test_add_class_trait_affects_existing_instances(self): + class A(HasTraits): + pass + + a = A() + + A.add_class_trait("y", Str()) + + self.assertEqual(a.y, "") + + def test_add_class_trait_affects_subclasses(self): + class A(HasTraits): + pass + + class B(A): + pass + + class C(B): + pass + + class D(B): + pass + + A.add_class_trait("y", Str()) + self.assertEqual(A().y, "") + self.assertEqual(B().y, "") + self.assertEqual(C().y, "") + self.assertEqual(D().y, "") + + def test_add_class_trait_has_items_and_subclasses(self): + # Regression test for enthought/traits#1460 + class A(HasTraits): + pass + + class B(A): + pass + + class C(B): + pass + + # Code branch for traits with items. + A.add_class_trait("x", List(Int)) + self.assertEqual(A().x, []) + self.assertEqual(B().x, []) + self.assertEqual(C().x, []) + + # Exercise the code branch for mapped traits. + A.add_class_trait("y", Map({"yes": 1, "no": 0}, default_value="no")) + self.assertEqual(A().y, "no") + self.assertEqual(B().y, "no") + self.assertEqual(C().y, "no") + + def test_add_class_trait_add_prefix_traits(self): + + class A(HasTraits): + pass + + A.add_class_trait("abc_", Str()) + A.add_class_trait("abc_def_", Int()) + + a = A() + self.assertEqual(a.abc_def_g, 0) + self.assertEqual(a.abc_z, "") + class TestObjectNotifiers(unittest.TestCase): """ Test calling object notifiers. """ From 87facf51084e5fabffccf6b5b112b724c1919646 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 12 May 2021 17:35:29 +0100 Subject: [PATCH 2/6] Add docstring for _add_class_trait --- traits/has_traits.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/traits/has_traits.py b/traits/has_traits.py index 2a3c0489c..613ca5ad5 100644 --- a/traits/has_traits.py +++ b/traits/has_traits.py @@ -1113,6 +1113,8 @@ def _trait_added_changed(self, name): def add_class_trait(cls, name, *trait): """ Adds a named trait attribute to this class. + Also adds the same attribute to all subclasses. + Parameters ---------- name : str @@ -1143,6 +1145,31 @@ def add_class_trait(cls, name, *trait): @classmethod def _add_class_trait(cls, name, trait, is_subclass): + """ + Add a named trait attribute to this class. + + Does not affect subclasses. + + Parameters + ---------- + name : str + Name of the attribute to add. + trait : CTrait + The trait to be added. + is_subclass : bool + True if we're adding the trait to a strict subclass of the + original class that add_class_trait was called for. This is used + to decide how to behave if ``cls`` already has a trait named + ``name``: in that circumstance, if ``is_subclass`` is False, an + error will be raised, while if ``is_subclass`` is True, no trait + will be added. + + Raises + ------ + TraitError + If a trait with the given name already exists, and is_subclass + is ``False``. + """ # Get a reference to the class's dictionary and 'prefix' traits: class_dict = cls.__dict__ prefix_traits = class_dict[PrefixTraits] From 020084c87b378c54b2683bf33b58196b2f3f2d84 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Wed, 12 May 2021 17:58:39 +0100 Subject: [PATCH 3/6] Fix mutation of HasTraits class by another test. --- traits/tests/test_abc.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/traits/tests/test_abc.py b/traits/tests/test_abc.py index 20b7c5b9b..239c2303b 100644 --- a/traits/tests/test_abc.py +++ b/traits/tests/test_abc.py @@ -29,8 +29,15 @@ def tearDown(self): warnings.filters[:] = self.old_filters def test_new(self): + # Previously, this test used HasTraits(x=10). That has the + # side-effect of creating an `x` trait on HasTraits, possibly + # causing interactions with other tests. + # xref: enthought/traits#58 + class A(HasTraits): + pass + # Should not raise DeprecationWarning. - HasTraits(x=10) + A(x=10) class AbstractFoo(ABCHasTraits): From 674de0f158f47481b2ecf9230ce2f192a0a2ede0 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Mon, 17 May 2021 09:57:01 +0100 Subject: [PATCH 4/6] Add tests for behaviour when the trait already exists --- traits/tests/test_has_traits.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/traits/tests/test_has_traits.py b/traits/tests/test_has_traits.py index edfa1eac6..5e6ce8326 100644 --- a/traits/tests/test_has_traits.py +++ b/traits/tests/test_has_traits.py @@ -35,6 +35,7 @@ ) from traits.traits import ForwardProperty, generic_trait from traits.trait_types import Event, Float, Instance, Int, List, Map, Str +from traits.trait_errors import TraitError def _dummy_getter(self): @@ -539,6 +540,30 @@ class A(HasTraits): self.assertEqual(a.abc_def_g, 0) self.assertEqual(a.abc_z, "") + def test_add_class_trait_when_trait_already_exists(self): + + class A(HasTraits): + foo = Int() + + with self.assertRaises(TraitError): + A.add_class_trait("foo", List()) + + self.assertEqual(A().foo, 0) + with self.assertRaises(AttributeError): + A().foo_items + + def test_add_class_trait_when_trait_already_exists_in_subclass(self): + class A(HasTraits): + pass + + class B(A): + foo = Int() + + A.add_class_trait("foo", Str()) + + self.assertEqual(A().foo, "") + self.assertEqual(B().foo, 0) + class TestObjectNotifiers(unittest.TestCase): """ Test calling object notifiers. """ From 0b8df3369bc1e09e3c1039a0125f10421d2a6a61 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Mon, 17 May 2021 10:11:18 +0100 Subject: [PATCH 5/6] More natural spelling for sort by decreasing length --- traits/has_traits.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traits/has_traits.py b/traits/has_traits.py index 613ca5ad5..650e66c05 100644 --- a/traits/has_traits.py +++ b/traits/has_traits.py @@ -643,7 +643,7 @@ def update_traits_class_dict(class_name, bases, class_dict): # Make sure the trait prefixes are sorted longest to shortest # so that we can easily bind dynamic traits to the longest matching # prefix: - prefix_list.sort(key=lambda x: -len(x)) + prefix_list.sort(key=len, reverse=True) # Get the list of all possible 'Instance'/'List(Instance)' handlers: instance_traits = _get_instance_handlers(class_dict, hastraits_bases) From ee805d49ab1fdbc8f5cf770e732c11543cf0b844 Mon Sep 17 00:00:00 2001 From: Mark Dickinson Date: Mon, 17 May 2021 10:11:39 +0100 Subject: [PATCH 6/6] Update traits/has_traits.py Co-authored-by: Corran Webster --- traits/has_traits.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traits/has_traits.py b/traits/has_traits.py index 650e66c05..b082c4754 100644 --- a/traits/has_traits.py +++ b/traits/has_traits.py @@ -1188,7 +1188,7 @@ def _add_class_trait(cls, name, trait, is_subclass): prefix_list.append(name) # Resort the list from longest to shortest: - prefix_list.sort(key=lambda x: -len(x)) + prefix_list.sort(key=len, reverse=True) return