From 0c46f18e7873048b7eb30eb9ba0395756e265917 2009-08-10 18:41:44 From: Brian Granger Date: 2009-08-10 18:41:44 Subject: [PATCH] Fixing subtle bug in the traitlets with This. Previously subclasses This traitlets wouldn't accept superclass instances as values. I have added a this_class attribute to TraitletType that is set by the metaclass and is used later by This.validate to properly handle this case. I have also added new tests for this. --- diff --git a/IPython/core/component.py b/IPython/core/component.py index e8f7e92..d86e60f 100644 --- a/IPython/core/component.py +++ b/IPython/core/component.py @@ -34,6 +34,9 @@ from IPython.utils.traitlets import ( #----------------------------------------------------------------------------- +class ComponentError(Exception): + pass + class MetaComponentTracker(type): """A metaclass that tracks instances of Components and its subclasses.""" @@ -116,12 +119,12 @@ class Component(HasTraitlets): __metaclass__ = MetaComponent # Traitlets are fun! - config = Instance(Struct) - parent = This(allow_none=True) - root = This(allow_none=True) + config = Instance(Struct,(),{}) + parent = This() + root = This() def __init__(self, parent, name=None, config=None): - """Create a component given a parent. + """Create a component given a parent and possibly and name and config. Parameters ---------- @@ -132,13 +135,26 @@ class Component(HasTraitlets): The unique name of the component. If empty, then a unique one will be autogenerated. config : Config - If this is empty, self.config = root.config, otherwise - self.config = config and root.config is ignored. This argument - should be used to pass the config to the root. Otherwise, it - can be used to *override* the inheritance of root.config. If a - caller wants to modify root.config (not override), the caller - should make a copy and change attributes and then pass the copy - to this argument. We might think about changing this behavior. + If this is empty, self.config = parent.config, otherwise + self.config = config and root.config is ignored. This argument + should only be used to *override* the automatic inheritance of + parent.config. If a caller wants to modify parent.config + (not override), the caller should make a copy and change + attributes and then pass the copy to this argument. + + Notes + ----- + Subclasses of Component must call the :meth:`__init__` method of + :class:`Component` *before* doing anything else and using + :func:`super`:: + + class MyComponent(Component): + def __init__(self, parent, name=None, config=None): + super(MyComponent, self).__init__(parent, name, config) + # Then any other code you need to finish initialization. + + This ensures that the :attr:`parent`, :attr:`name` and :attr:`config` + attributes are handled properly. """ super(Component, self).__init__() self._children = [] @@ -169,6 +185,15 @@ class Component(HasTraitlets): else: self.root = new.root + def _root_changed(self, name, old, new): + if self.parent is None: + if not (new is self): + raise ComponentError("Root not self, but parent is None.") + else: + if not self.parent.root is new: + raise ComponentError("Error in setting the root attribute: " + "root != parent.root") + @property def children(self): """A list of all my child components.""" diff --git a/IPython/core/tests/test_component.py b/IPython/core/tests/test_component.py index e1bafa2..ebcb61a 100644 --- a/IPython/core/tests/test_component.py +++ b/IPython/core/tests/test_component.py @@ -22,7 +22,11 @@ Authors: from unittest import TestCase -from IPython.core.component import Component +from IPython.core.component import Component, ComponentError +from IPython.utils.traitlets import ( + TraitletError +) +from IPython.utils.ipstruct import Struct #----------------------------------------------------------------------------- @@ -39,6 +43,30 @@ class TestComponentMeta(TestCase): c2 = BaseComponent(c1) self.assertEquals(BaseComponent.get_instances(),[c1,c2]) + def test_get_instances_subclass(self): + class MyComponent(Component): + pass + class MyOtherComponent(MyComponent): + pass + c1 = MyComponent(None) + c2 = MyOtherComponent(c1) + c3 = MyOtherComponent(c2) + self.assertEquals(MyComponent.get_instances(), [c1, c2, c3]) + self.assertEquals(MyComponent.get_instances(klass=MyOtherComponent), [c2, c3]) + + def test_get_instances_root(self): + class MyComponent(Component): + pass + class MyOtherComponent(MyComponent): + pass + c1 = MyComponent(None) + c2 = MyOtherComponent(c1) + c3 = MyOtherComponent(c2) + c4 = MyComponent(None) + c5 = MyComponent(c4) + self.assertEquals(MyComponent.get_instances(root=c1), [c1, c2, c3]) + self.assertEquals(MyComponent.get_instances(root=c4), [c4, c5]) + class TestComponent(TestCase): @@ -65,3 +93,78 @@ class TestComponent(TestCase): self.assertEquals(c2.root, c1) self.assertEquals(c3.root, c1) self.assertEquals(c4.root, c1) + + def test_change_parent(self): + c1 = Component(None) + c2 = Component(None) + c3 = Component(c1) + self.assertEquals(c3.root, c1) + self.assertEquals(c3.parent, c1) + self.assertEquals(c1.children,[c3]) + c3.parent = c2 + self.assertEquals(c3.root, c2) + self.assertEquals(c3.parent, c2) + self.assertEquals(c2.children,[c3]) + self.assertEquals(c1.children,[]) + + def test_subclass_parent(self): + c1 = Component(None) + self.assertRaises(TraitletError, setattr, c1, 'parent', 10) + + class MyComponent(Component): + pass + c1 = Component(None) + c2 = MyComponent(c1) + self.assertEquals(MyComponent.parent.this_class, Component) + self.assertEquals(c2.parent, c1) + + def test_bad_root(self): + c1 = Component(None) + c2 = Component(None) + c3 = Component(None) + self.assertRaises(ComponentError, setattr, c1, 'root', c2) + c1.parent = c2 + self.assertEquals(c1.root, c2) + self.assertRaises(ComponentError, setattr, c1, 'root', c3) + + +class TestComponentConfig(TestCase): + + def test_default(self): + c1 = Component(None) + c2 = Component(c1) + c3 = Component(c2) + self.assertEquals(c1.config, c2.config) + self.assertEquals(c2.config, c3.config) + + def test_custom(self): + config = Struct() + config.FOO = 'foo' + config.BAR = 'bar' + c1 = Component(None, config=config) + c2 = Component(c1) + c3 = Component(c2) + self.assertEquals(c1.config, config) + self.assertEquals(c2.config, config) + self.assertEquals(c3.config, config) + +class TestComponentName(TestCase): + + def test_default(self): + class MyComponent(Component): + pass + c1 = Component(None) + c2 = MyComponent(None) + c3 = Component(c2) + self.assertNotEquals(c1.name, c2.name) + self.assertNotEquals(c1.name, c3.name) + + def test_manual(self): + class MyComponent(Component): + pass + c1 = Component(None, name='foo') + c2 = MyComponent(None, name='bar') + c3 = Component(c2, name='bah') + self.assertEquals(c1.name, 'foo') + self.assertEquals(c2.name, 'bar') + self.assertEquals(c3.name, 'bah') diff --git a/IPython/utils/tests/test_traitlets.py b/IPython/utils/tests/test_traitlets.py index ce1f76d..d51f5ee 100644 --- a/IPython/utils/tests/test_traitlets.py +++ b/IPython/utils/tests/test_traitlets.py @@ -164,6 +164,17 @@ class TestHasTraitletsMeta(TestCase): c.c = 10 self.assertEquals(c.c,10) + def test_this_class(self): + class A(HasTraitlets): + t = This() + tt = This() + class B(A): + tt = This() + ttt = This() + self.assertEquals(A.t.this_class, A) + self.assertEquals(B.t.this_class, A) + self.assertEquals(B.tt.this_class, B) + self.assertEquals(B.ttt.this_class, B) class TestHasTraitletsNotify(TestCase): @@ -323,7 +334,7 @@ class TestTraitletKeys(TestCase): a = Int b = Float a = A() - self.assertEquals(a.traitlet_keys(),['a','b']) + self.assertEquals(a.traitlet_names(),['a','b']) #----------------------------------------------------------------------------- @@ -484,6 +495,28 @@ class TestThis(TestCase): f.this = Foo() self.assert_(isinstance(f.this, Foo)) + def test_subclass(self): + class Foo(HasTraitlets): + t = This() + class Bar(Foo): + pass + f = Foo() + b = Bar() + f.t = b + b.t = f + self.assertEquals(f.t, b) + self.assertEquals(b.t, f) + + def test_subclass_override(self): + class Foo(HasTraitlets): + t = This() + class Bar(Foo): + t = This() + f = Foo() + b = Bar() + f.t = b + self.assertEquals(f.t, b) + self.assertRaises(TraitletError, setattr, b, 't', f) class TraitletTestBase(TestCase): """A best testing class for basic traitlet types.""" diff --git a/IPython/utils/traitlets.py b/IPython/utils/traitlets.py index cd30fe3..f83ea9f 100644 --- a/IPython/utils/traitlets.py +++ b/IPython/utils/traitlets.py @@ -139,6 +139,23 @@ def parse_notifier_name(name): class TraitletType(object): + """A base class for all traitlet descriptors. + + Notes + ----- + Our implementation of traitlets is based on Python's descriptor + prototol. This class is the base class for all such descriptors. The + only magic we use is a custom metaclass for the main :class:`HasTraitlets` + class that does the following: + + 1. Sets the :attr:`name` attribute of every :class:`TraitletType` + instance in the class dict to the name of the attribute. + 2. Sets the :attr:`this_class` attribute of every :class:`TraitletType` + instance in the class dict to the *class* that declared the traitlet. + This is used by the :class:`This` traitlet to allow subclasses to + accept superclasses for :class:`This` values. + """ + metadata = {} default_value = Undefined @@ -235,6 +252,17 @@ class MetaHasTraitlets(type): """ def __new__(mcls, name, bases, classdict): + """Create the HasTraitlets class. + + This instantiates all TraitletTypes in the class dict and sets their + :attr:`name` attribute. + """ + # print "=========================" + # print "MetaHasTraitlets.__new__" + # print "mcls, ", mcls + # print "name, ", name + # print "bases, ", bases + # print "classdict, ", classdict for k,v in classdict.iteritems(): if isinstance(v, TraitletType): v.name = k @@ -245,6 +273,22 @@ class MetaHasTraitlets(type): classdict[k] = vinst return super(MetaHasTraitlets, mcls).__new__(mcls, name, bases, classdict) + def __init__(cls, name, bases, classdict): + """Finish initializing the HasTraitlets class. + + This sets the :attr:`this_class` attribute of each TraitletType in the + class dict to the newly created class ``cls``. + """ + # print "=========================" + # print "MetaHasTraitlets.__init__" + # print "cls, ", cls + # print "name, ", name + # print "bases, ", bases + # print "classdict, ", classdict + for k, v in classdict.iteritems(): + if isinstance(v, TraitletType): + v.this_class = cls + super(MetaHasTraitlets, cls).__init__(name, bases, classdict) class HasTraitlets(object): @@ -364,7 +408,7 @@ class HasTraitlets(object): for n in names: self._add_notifiers(handler, n) - def traitlet_keys(self): + def traitlet_names(self): """Get a list of all the names of this classes traitlets.""" return [memb[0] for memb in inspect.getmembers(self.__class__) if isinstance(memb[1], TraitletType)] @@ -566,7 +610,10 @@ class This(ClassBasedTraitletType): super(This, self).__init__(None, **metadata) def validate(self, obj, value): - if isinstance(value, obj.__class__) or (value is None): + # What if value is a superclass of obj.__class__? This is + # complicated if it was the superclass that defined the This + # traitlet. + if isinstance(value, self.this_class) or (value is None): return value else: self.error(obj, value)