Repository: ipython/traitlets
Pull request #325 (2016-09-26 08:55:04)

avoid modifying inherited class attr
could affect parent classes incorrectly.


Patch:
diff --git a/.travis.yml b/.travis.yml
index 362ed0d..dc5887d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,11 +1,9 @@
 language: python
 python:
-    - "nightly"
     - 3.5
     - 3.4
     - 2.7
     - 3.3
-    - 3.6-dev
 sudo: false
 install:
     - pip install .
@@ -14,6 +12,3 @@ script:
     - py.test --cov traitlets -v traitlets
 after_success:
     - codecov
-matrix:
-    allow_failures:
-        - python: "nightly"
diff --git a/CHANGES.txt b/CHANGES.txt
deleted file mode 100644
index 8cea6e7..0000000
--- a/CHANGES.txt
+++ /dev/null
@@ -1,1 +0,0 @@
-The changes are at "./docs/source/changelog.rst".
diff --git a/docs/source/using_traitlets.rst b/docs/source/using_traitlets.rst
index 8dada48..7e2206d 100644
--- a/docs/source/using_traitlets.rst
+++ b/docs/source/using_traitlets.rst
@@ -76,18 +76,10 @@ When observers are methods of the class, a decorator syntax can be used.
             print(change['old'])
             print(change['new'])
 
-Validation and Coercion
------------------------
+Validation
+----------
 
-Custom Cross-Validation
-^^^^^^^^^^^^^^^^^^^^^^^
-
-Each trait type (``Int``, ``Unicode``, ``Dict`` etc.) may have its own
-validation or coercion logic. In addition, we can register custom
-cross-validators that may depend on the state of other attributes.
-
-Basic Example: Validating the Parity of a Trait
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Custom validation logic on trait classes
 
 .. code:: python
 
@@ -119,81 +111,12 @@ Basic Example: Validating the Parity of a Trait
         parity_check.value = 1
         parity_check.parity = 1
 
-However, we recommend that custom cross-validators don't modify the state of
-the HasTraits instance.
-
-Advanced Example: Validating the Schema
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-The ``List`` and ``Dict`` trait types allow the validation of nested
-properties.
-
-.. code:: python
-
-    from traitlets import HasTraits, Dict, Bool, Unicode
-
-    class Nested(HasTraits):
-
-        value = Dict(traits={
-            'configuration': Dict(trait=Unicode()),
-            'flag': Bool()
-        })
-
-    n = Nested()
-    n.value = dict(flag=True, configuration={})  # OK
-    n.value = dict(flag=True, configuration='')  # raises a TraitError.
-
-
-However, for deeply nested properties it might be more appropriate to use an
-external validator:
-
-.. code:: python
-
-    import jsonschema
-
-    value_schema = {
-         'type' : 'object',
-         'properties' : {
-             'price' : { 'type' : 'number' },
-             'name' : { 'type' : 'string' },
-         },
-     }
-
-    from traitlets import HasTraits, Dict, TraitError, validate, default
-
-    class Schema(HasTraits):
-
-        value = Dict()
-
-        @default('value')
-        def _default_value(self):
-            return dict(name='', price=1)
-
-        @validate('value')
-        def _validate_value(self, proposal):
-            try:
-                jsonschema.validate(proposal['value'], value_schema)
-            except jsonschema.ValidationError as e:
-                raise TraitError(e)
-            return proposal['value']
-
-    s = Schema()
-    s.value = dict(name='', price='1')  # raises a TraitError
-
-
-Holding Trait Cross-Validation and Notifications
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-Sometimes if may be impossible to transition from to valid states for a
-``HasTraits`` instance by change attributes one by one. The
-``hold_trait_notifications`` context manager can be used to hold the custom
-cross validation until the context manager is released. If a validation error
-occurs, changes are rolled back to the initial state.
-
-Custom Events
--------------
+In the case where the a validation error occurs when
+``hold_trait_notifications`` context manager is released, changes are
+rolled back to the initial state.
 
-Finally, trait types can emit other events types than trait changes. This
-capability was added so as to enable notifications on change of values in
-container classes. The items available in the dictionary passed to the observer
-registered with ``observe`` depends on the event type.
+-  Finally, trait type can have other events than trait changes. This
+   capability was added so as to enable notifications on change of
+   values in container classes. The items available in the dictionary
+   passed to the observer registered with ``observe`` depends on the
+   event type.
diff --git a/traitlets/config/application.py b/traitlets/config/application.py
index b79062b..e1e8baf 100644
--- a/traitlets/config/application.py
+++ b/traitlets/config/application.py
@@ -272,8 +272,13 @@ class Application(SingletonConfigurable):
         SingletonConfigurable.__init__(self, **kwargs)
         # Ensure my class is in self.classes, so my attributes appear in command line
         # options and config files.
-        if self.__class__ not in self.classes:
-            self.classes.insert(0, self.__class__)
+        cls = self.__class__
+        if cls not in self.classes:
+            if self.classes is cls.classes:
+                # class attr, assign instead of insert
+                cls.classes = [cls] + self.classes
+            else:
+                self.classes.insert(0, self.__class__)
 
     @observe('config')
     @observe_compat
@@ -312,28 +317,16 @@ class Application(SingletonConfigurable):
                 classdict[c.__name__] = c
 
         for alias, longname in self.aliases.items():
-            try:
-                classname, traitname = longname.split('.',1)
-                cls = classdict[classname]
-
-                trait = cls.class_traits(config=True)[traitname]
-                fhelp = cls.class_get_trait_help(trait).splitlines()
-
-                if not isinstance(alias, tuple):
-                    alias = (alias, )
-                alias = sorted(alias, key=len)
-                alias = ', '.join(('--%s' if len(m) > 1 else '-%s') % m
-                                  for m in alias)
-
-                # reformat first line
-                fhelp[0] = fhelp[0].replace('--'+longname, alias)
-                lines.extend(fhelp)
-                lines.append(indent("Equivalent to: [--%s]" % longname))
-            except Exception as ex:
-                self.log.error('Failed collecting help-message for alias %r, due to: %s',
-                               alias, ex)
-                raise
-
+            classname, traitname = longname.split('.',1)
+            cls = classdict[classname]
+
+            trait = cls.class_traits(config=True)[traitname]
+            help = cls.class_get_trait_help(trait).splitlines()
+            # reformat first line
+            help[0] = help[0].replace(longname, alias) + ' (%s)'%longname
+            if len(alias) == 1:
+                help[0] = help[0].replace('--%s='%alias, '-%s '%alias)
+            lines.extend(help)
         # lines.append('')
         print(os.linesep.join(lines))
 
@@ -343,25 +336,10 @@ class Application(SingletonConfigurable):
             return
 
         lines = []
-        for flags, (cfg, fhelp) in self.flags.items():
-            try:
-                if not isinstance(flags, tuple):
-                    flags = (flags, )
-                flags = sorted(flags, key=len)
-                flags = ', '.join(('--%s' if len(m) > 1 else '-%s') % m
-                              for m in flags)
-                lines.append(flags)
-                lines.append(indent(dedent(fhelp.strip())))
-                cfg_list = ' '.join('--%s.%s=%s' %(clname, prop, val)
-                            for clname, props_dict
-                            in cfg.items() for prop, val in props_dict.items())
-                cfg_txt = "Equivalent to: [%s]" % cfg_list
-                lines.append(indent(dedent(cfg_txt)))
-            except Exception as ex:
-                self.log.error('Failed collecting help-message for flag %r, due to: %s',
-                               flags, ex)
-                raise
-
+        for m, (cfg,help) in self.flags.items():
+            prefix = '--' if len(m) > 1 else '-'
+            lines.append(prefix+m)
+            lines.append(indent(dedent(help.strip())))
         # lines.append('')
         print(os.linesep.join(lines))
 
@@ -408,7 +386,7 @@ class Application(SingletonConfigurable):
         self.print_options()
 
         if classes:
-            help_classes = self._classes_in_config_sample()
+            help_classes = self.classes
             if help_classes:
                 print("Class parameters")
                 print("----------------")
@@ -502,10 +480,7 @@ class Application(SingletonConfigurable):
             if len(children) == 1:
                 # exactly one descendent, promote alias
                 cls = children[0]
-            if not isinstance(aliases, tuple):
-                alias = (alias, )
-            for al in alias:
-                aliases[al] = '.'.join([cls,trait])
+            aliases[alias] = '.'.join([cls,trait])
 
         # flatten flags, which are of the form:
         # { 'key' : ({'Cls' : {'trait' : value}}, 'help')}
@@ -518,10 +493,7 @@ class Application(SingletonConfigurable):
                 if len(children) == 1:
                     cls = children[0]
                 newflag[cls] = subdict
-            if not isinstance(key, tuple):
-                key = (key, )
-            for k in key:
-                flags[k] = (newflag, help)
+            flags[key] = (newflag, help)
         return flags, aliases
 
     @catch_config_error
@@ -580,7 +552,7 @@ class Application(SingletonConfigurable):
             # path list is in descending priority order, so load files backwards:
             pyloader = cls.python_config_loader_class(basefilename+'.py', path=path, log=log)
             if log:
-                log.debug("Looking for %s in %s", basefilename, path or os.getcwd())
+                log.debug("Looking for %s in %s", basefilename, path)
             jsonloader = cls.json_config_loader_class(basefilename+'.json', path=path, log=log)
             config = None
             loaded = []
@@ -614,7 +586,7 @@ class Application(SingletonConfigurable):
                     yield config
                     loaded.append(config)
                     filenames.append(loader.full_filename)
-
+            
 
 
     @catch_config_error
diff --git a/traitlets/config/configurable.py b/traitlets/config/configurable.py
index bbc1060..1174fcf 100644
--- a/traitlets/config/configurable.py
+++ b/traitlets/config/configurable.py
@@ -210,8 +210,7 @@ class Configurable(HasTraits):
         """
         assert inst is None or isinstance(inst, cls)
         final_help = []
-        base_classes = ','.join(p.__name__ for p in cls.__bases__)
-        final_help.append(u'%s(%s) options' % (cls.__name__, base_classes))
+        final_help.append(u'%s options' % cls.__name__)
         final_help.append(len(final_help[0])*u'-')
         for k, v in sorted(cls.class_traits(config=True).items()):
             help = cls.class_get_trait_help(v, inst)
diff --git a/traitlets/config/loader.py b/traitlets/config/loader.py
index 3a75350..803b362 100644
--- a/traitlets/config/loader.py
+++ b/traitlets/config/loader.py
@@ -786,31 +786,25 @@ class KVArgParseConfigLoader(ArgParseConfigLoader):
         if flags is None:
             flags = self.flags
         paa = self.parser.add_argument
-        for keys,value in aliases.items():
-            if not isinstance(keys, tuple):
-                keys = (keys, )
-            for key in keys:
-                if key in flags:
-                    # flags
-                    nargs = '?'
-                else:
-                    nargs = None
-                if len(key) is 1:
-                    paa('-'+key, '--'+key, type=text_type, dest=value, nargs=nargs)
-                else:
-                    paa('--'+key, type=text_type, dest=value, nargs=nargs)
-        for keys, (value, help) in flags.items():
-            if not isinstance(keys, tuple):
-                keys = (keys, )
-            for key in keys:
-                if key in self.aliases:
-                    #
-                    self.alias_flags[self.aliases[key]] = value
-                    continue
-                if len(key) is 1:
-                    paa('-'+key, '--'+key, action='append_const', dest='_flags', const=value)
-                else:
-                    paa('--'+key, action='append_const', dest='_flags', const=value)
+        for key,value in aliases.items():
+            if key in flags:
+                # flags
+                nargs = '?'
+            else:
+                nargs = None
+            if len(key) is 1:
+                paa('-'+key, '--'+key, type=text_type, dest=value, nargs=nargs)
+            else:
+                paa('--'+key, type=text_type, dest=value, nargs=nargs)
+        for key, (value, help) in flags.items():
+            if key in self.aliases:
+                #
+                self.alias_flags[self.aliases[key]] = value
+                continue
+            if len(key) is 1:
+                paa('-'+key, '--'+key, action='append_const', dest='_flags', const=value)
+            else:
+                paa('--'+key, action='append_const', dest='_flags', const=value)
 
     def _convert_to_config(self):
         """self.parsed_data->self.config, parse unrecognized extra args via KVLoader."""
diff --git a/traitlets/traitlets.py b/traitlets/traitlets.py
index af963d9..c23151d 100644
--- a/traitlets/traitlets.py
+++ b/traitlets/traitlets.py
@@ -1119,10 +1119,16 @@ class HasTraits(six.with_metaclass(MetaHasTraits, HasDescriptors)):
                 cache = {}
                 raise e
             finally:
+                # Reset the notify_change to original value, enable cross-validation
+                # and fire resulting change notifications.
+                self.notify_change = notify_change
                 self._cross_validation_lock = False
-                # Restore method retrieval from class
-                del self.notify_change
 
+                if isinstance(notify_change, types.MethodType):
+                    # Presence of the notify_change method
+                    # on __dict__ can cause memory leaks
+                    # and prevents pickleability
+                    self.__dict__.pop('notify_change')
                 # trigger delayed notifications
                 for changes in cache.values():
                     for change in changes:
@@ -1375,7 +1381,7 @@ class HasTraits(six.with_metaclass(MetaHasTraits, HasDescriptors)):
         result = {}
         for name, trait in traits.items():
             for meta_name, meta_eval in metadata.items():
-                if not callable(meta_eval):
+                if type(meta_eval) is not types.FunctionType:
                     meta_eval = _SimpleTest(meta_eval)
                 if not meta_eval(trait.metadata.get(meta_name, None)):
                     break
@@ -1425,7 +1431,7 @@ class HasTraits(six.with_metaclass(MetaHasTraits, HasDescriptors)):
         result = {}
         for name, trait in traits.items():
             for meta_name, meta_eval in metadata.items():
-                if not callable(meta_eval):
+                if type(meta_eval) is not types.FunctionType:
                     meta_eval = _SimpleTest(meta_eval)
                 if not meta_eval(trait.metadata.get(meta_name, None)):
                     break


Test patch:
diff --git a/traitlets/config/tests/test_application.py b/traitlets/config/tests/test_application.py
index dcc3c2f..5db0243 100644
--- a/traitlets/config/tests/test_application.py
+++ b/traitlets/config/tests/test_application.py
@@ -6,12 +6,9 @@ Tests for traitlets.config.application.Application
 # Copyright (c) IPython Development Team.
 # Distributed under the terms of the Modified BSD License.
 
-import contextlib
-import io
 import json
 import logging
 import os
-import sys
 from io import StringIO
 from unittest import TestCase
 
@@ -37,6 +34,7 @@ from traitlets.traitlets import (
     Bool, Unicode, Integer, List, Dict
 )
 
+
 class Foo(Configurable):
 
     i = Integer(0, help="The integer i.").tag(config=True)
@@ -61,23 +59,18 @@ class MyApp(Application):
             help="Should print a warning if `MyApp.warn-typo=...` command is passed")
 
     aliases = Dict({
-                    ('fooi', 'i') : 'Foo.i',
-                    ('j', 'fooj') : 'Foo.j',
+                    'i' : 'Foo.i',
+                    'j' : 'Foo.j',
                     'name' : 'Foo.name',
                     'enabled' : 'Bar.enabled',
                     'log-level' : 'Application.log_level',
                 })
 
-    flags = Dict({('enable', 'e'):
-                        ({'Bar': {'enabled' : True}},
-                         "Set Bar.enabled to True"),
-                  ('d', 'disable'):
-                        ({'Bar': {'enabled' : False}},
-                         "Set Bar.enabled to False"),
-                  'crit':
-                        ({'Application' : {'log_level' : logging.CRITICAL}},
+    flags = Dict(dict(enable=({'Bar': {'enabled' : True}}, "Set Bar.enabled to True"),
+                  disable=({'Bar': {'enabled' : False}}, "Set Bar.enabled to False"),
+                  crit=({'Application' : {'log_level' : logging.CRITICAL}},
                         "set level=CRITICAL"),
-            })
+            ))
 
     def init_foo(self):
         self.foo = Foo(parent=self)
@@ -190,35 +183,10 @@ class TestApplication(TestCase):
         app.parse_command_line(["--disable"])
         app.init_bar()
         self.assertEqual(app.bar.enabled, False)
-
-        app = MyApp()
-        app.parse_command_line(["-d"])
-        app.init_bar()
-        self.assertEqual(app.bar.enabled, False)
-
-        app = MyApp()
         app.parse_command_line(["--enable"])
         app.init_bar()
         self.assertEqual(app.bar.enabled, True)
 
-        app = MyApp()
-        app.parse_command_line(["-e"])
-        app.init_bar()
-        self.assertEqual(app.bar.enabled, True)
-
-    @mark.skipif(sys.version_info < (3, 4),
-                 reason="Missing `contextlib.redirect_stdout` in python < 3.4!")
-    def test_flags_help_msg(self):
-        app = MyApp()
-        stdout = io.StringIO()
-        with contextlib.redirect_stdout(stdout):
-            app.print_flag_help()
-        hmsg = stdout.getvalue()
-        self.assertRegex(hmsg, "(?<!-)-e, --enable\\b")
-        self.assertRegex(hmsg, "(?<!-)-d, --disable\\b")
-        self.assertIn("Equivalent to: [--Bar.enabled=True]", hmsg)
-        self.assertIn("Equivalent to: [--Bar.enabled=False]", hmsg)
-
     def test_aliases(self):
         app = MyApp()
         app.parse_command_line(["--i=5", "--j=10"])
@@ -227,34 +195,6 @@ class TestApplication(TestCase):
         app.init_foo()
         self.assertEqual(app.foo.j, 10)
 
-        app = MyApp()
-        app.parse_command_line(["-i=5", "-j=10"])
-        app.init_foo()
-        self.assertEqual(app.foo.i, 5)
-        app.init_foo()
-        self.assertEqual(app.foo.j, 10)
-
-        app = MyApp()
-        app.parse_command_line(["--fooi=5", "--fooj=10"])
-        app.init_foo()
-        self.assertEqual(app.foo.i, 5)
-        app.init_foo()
-        self.assertEqual(app.foo.j, 10)
-
-    @mark.skipif(sys.version_info < (3, 4),
-                 reason="Missing `contextlib.redirect_stdout` in python < 3.4!")
-    def test_aliases_help_msg(self):
-        app = MyApp()
-        stdout = io.StringIO()
-        with contextlib.redirect_stdout(stdout):
-            app.print_alias_help()
-        hmsg = stdout.getvalue()
-        self.assertRegex(hmsg, "(?<!-)-i, --fooi\\b")
-        self.assertRegex(hmsg, "(?<!-)-j, --fooj\\b")
-        self.assertIn("Equivalent to: [--Foo.i]", hmsg)
-        self.assertIn("Equivalent to: [--Foo.j]", hmsg)
-        self.assertIn("Equivalent to: [--Foo.name]", hmsg)
-
     def test_flag_clobber(self):
         """test that setting flags doesn't clobber existing settings"""
         app = MyApp()
diff --git a/traitlets/config/tests/test_configurable.py b/traitlets/config/tests/test_configurable.py
index fb2e2d5..cf269c0 100644
--- a/traitlets/config/tests/test_configurable.py
+++ b/traitlets/config/tests/test_configurable.py
@@ -31,8 +31,8 @@ class MyConfigurable(Configurable):
     c = Unicode('no config')
 
 
-mc_help=u"""MyConfigurable(Configurable) options
-------------------------------------
+mc_help=u"""MyConfigurable options
+----------------------
 --MyConfigurable.a=<Integer>
     Default: 1
     The integer a.
@@ -40,8 +40,8 @@ mc_help=u"""MyConfigurable(Configurable) options
     Default: 1.0
     The integer b."""
 
-mc_help_inst=u"""MyConfigurable(Configurable) options
-------------------------------------
+mc_help_inst=u"""MyConfigurable options
+----------------------
 --MyConfigurable.a=<Integer>
     Current: 5
     The integer a.
@@ -89,7 +89,7 @@ class TestConfigurable(TestCase):
         self.assertTrue(c3.config is config)
         self.assertTrue(c1.config is c2.config)
         self.assertTrue(c2.config is c3.config)
-
+        
     def test_inheritance(self):
         config = Config()
         config.MyConfigurable.a = 2
@@ -179,7 +179,7 @@ class MyParent2(MyParent):
     pass
 
 class TestParentConfigurable(TestCase):
-
+    
     def test_parent_config(self):
         cfg = Config({
             'MyParent' : {
@@ -275,11 +275,11 @@ class Containers(Configurable):
     lis = List().tag(config=True)
     def _lis_default(self):
         return [-1]
-
+    
     s = Set().tag(config=True)
     def _s_default(self):
         return {'a'}
-
+    
     d = Dict().tag(config=True)
     def _d_default(self):
         return {'a' : 'b'}
@@ -346,22 +346,22 @@ class TestConfigContainers(TestCase):
         c.Containers.d.update({'e' : 'f'})
         obj = Containers(config=c)
         self.assertEqual(obj.d, {'a':'b', 'c':'d', 'e':'f'})
-
+    
     def test_update_twice(self):
         c = Config()
         c.MyConfigurable.a = 5
         m = MyConfigurable(config=c)
         self.assertEqual(m.a, 5)
-
+        
         c2 = Config()
         c2.MyConfigurable.a = 10
         m.update_config(c2)
         self.assertEqual(m.a, 10)
-
+        
         c2.MyConfigurable.a = 15
         m.update_config(c2)
         self.assertEqual(m.a, 15)
-
+    
     def test_update_self(self):
         """update_config with same config object still triggers config_changed"""
         c = Config()
@@ -371,7 +371,7 @@ class TestConfigContainers(TestCase):
         c.MyConfigurable.a = 10
         m.update_config(c)
         self.assertEqual(m.a, 10)
-
+    
     def test_config_default(self):
         class SomeSingleton(SingletonConfigurable):
             pass
@@ -388,9 +388,9 @@ class TestConfigContainers(TestCase):
 
         d1 = DefaultConfigurable()
         self.assertEqual(d1.a, 0)
-
+        
         single = SomeSingleton.instance(config=c)
-
+        
         d2 = DefaultConfigurable()
         self.assertIs(d2.config, single.config)
         self.assertEqual(d2.a, 5)
@@ -415,9 +415,9 @@ class TestConfigContainers(TestCase):
 
         d1 = DefaultConfigurable()
         self.assertEqual(d1.a, 0)
-
+        
         single = SomeSingleton.instance(config=c)
-
+        
         d2 = DefaultConfigurable()
         self.assertIs(d2.config, single.config)
         self.assertEqual(d2.a, 5)
@@ -429,14 +429,14 @@ class TestLogger(TestCase):
             foo = Integer(config=True)
             bar = Integer(config=True)
             baz = Integer(config=True)
-
+    
     @mark.skipif(not hasattr(TestCase, 'assertLogs'), reason='requires TestCase.assertLogs')
     def test_warn_match(self):
         logger = logging.getLogger('test_warn_match')
         cfg = Config({'A': {'bat': 5}})
         with self.assertLogs(logger, logging.WARNING) as captured:
             a = TestLogger.A(config=cfg, log=logger)
-
+        
         output = '\n'.join(captured.output)
         self.assertIn('Did you mean one of: `bar, baz`?', output)
         self.assertIn('Config option `bat` not recognized by `A`.', output)
@@ -444,7 +444,7 @@ class TestLogger(TestCase):
         cfg = Config({'A': {'fool': 5}})
         with self.assertLogs(logger, logging.WARNING) as captured:
             a = TestLogger.A(config=cfg, log=logger)
-
+        
         output = '\n'.join(captured.output)
         self.assertIn('Config option `fool` not recognized by `A`.', output)
         self.assertIn('Did you mean `foo`?', output)