Repository: ipython/traitlets
Pull request #385 (2017-03-24 12:21:50)

generate all config, not just new traits on each class
using class_own_traits splits up config for classes with parents, and hides some config options entirely if traits are defined in base classes not in the final `classes` list.

The change has not been released, so this restores the behavior of all released traitlets versions.

ankostis: > using `class_own_traits` splits up config for classes with parents, 

Yes, that is intentional by #289, to deduce clutter when generating help and sample-config files with non-trivial hierarchies of Configurables.  And to compensate the "split", that PR added the base-classes in the output, to help user decide in which class to set a config-param.

To give an example of why I had crafted that PR, in [our application ](/JRCSTU/CO2MPAS-TA/tree/master/co2mpas/sampling), it reduced the number of lines of the generated sample configuration file by a factor of x5:

$ wc -l config-PR385.py config-PR289.py
  3578 config-PR385.py
   729 config-PR289.py

The extra lines are identical copies of help-messages from traits included in subclasses.

So would it be possible to identify the root problem instead of going back to old behavior?


> and hides some config options entirely if traits are defined in base classes not in the final classes list.

If that is happening, that's a certain bug: adding a subclass in the `classes` list should be enough for all baseclass traits of to appear *somewhere* in the help-messages.
ankostis: A possible fix would be not to include the help-message again on each subclass but just refere to the baseclass?
minrk: I realize it was intentional, but I discovered after using it that the result is a significant regression. 

> So would it be possible to identify the root problem instead of going back to old behavior?

When generating config for an object, I want to see everything I can configure on that object in one place. Relying on references and inheritance means that I have to look in several different disconnected places in the file in order to see the configuration of one object. So the important thing for me is that when I see the section of a config file corresponding to one object, I get every configurable for that object.

Since this is a regression from previous releases, perhaps we could make the 'only not inherited' behavior n off-by-default option?
ankostis: Certainly adding a flag is doable, but maybe the following 2 alternatives improve the UI experience:

a. Not repeating help messages on each config-param copy.
b. Collecting all sub-class parameters beneath the 1st metnion of the base-class param.

If you think it is worth it, instead of a flag, I can craft some mockups here for config-files/help msgs to compare and decide, should I?
minrk: > If you think it is worth it, instead of a flag, I can craft some mockups here for config-files/help msgs to compare and decide, should I?

sure, that would be great!

The most important thing for me is that when I see a class's entry in a config file, I see every configurable trait for that class. There does need to be *some* explanation of it, at least. How's this for a compromise:

- always show all configurable traits for every class
- only show *full* description on defining class
- on subclasses, show first line of help and originating Class.trait name for easy reference to original
- Fix bug: ensure defining class is always included in the config file
minrk: Here's the pattern I found that prompted this PR:

A collection of configurable classes with the same/similar config options inherited from a Mixin. That Mixin is intended to be an implementation detail, and shouldn't affect the config file. In a typical config file, only one of these classes will be configured, so the norm is to find the section corresponding to your chosen class and configure it. In master, all of these classes produce empty config sections, which is not a good experience, and the Mixin defining the configurable traits isn't present, so it is impossible to discover what is configurable.

The best experience in these cases would be for every class in the `classes` list to get all of its config with all help in one place. It would be preferable to avoid including the Mixin class in the config file, and it would be preferable to avoid having 'references' to the originating classes, which are excluded from `classes` on purpose.

So something like this should get the right result:

- identify 'defining' class for a given trait as a class whose parent is not in the `classes` list
- truncate help to one line + "See Original.trait..." if the defining class is elsewhere in the config file

Patch:
diff --git a/traitlets/config/application.py b/traitlets/config/application.py
index 45183cf..708ed53 100644
--- a/traitlets/config/application.py
+++ b/traitlets/config/application.py
@@ -283,7 +283,7 @@ class Application(SingletonConfigurable):
         if cls not in self.classes:
             if self.classes is cls.classes:
                 # class attr, assign instead of insert
-                cls.classes = [cls] + self.classes
+                self.classes = [cls] + self.classes
             else:
                 self.classes.insert(0, self.__class__)
 
@@ -648,8 +648,6 @@ class Application(SingletonConfigurable):
                     loaded.append(config)
                     filenames.append(loader.full_filename)
 
-
-
     @catch_config_error
     def load_config_file(self, filename, path=None):
         """Load config files by filename and path."""
@@ -663,10 +661,9 @@ class Application(SingletonConfigurable):
         new_config.merge(self.cli_config)
         self.update_config(new_config)
 
-
     def _classes_with_config_traits(self, classes=None):
         """
-        Yields only classes with own traits, and their subclasses.
+        Yields only classes with configurable traits, and their subclasses.
 
         :param classes:
             The list of classes to iterate; if not set, uses :attr:`classes`.
@@ -702,12 +699,14 @@ class Application(SingletonConfigurable):
             if inc_yes:
                 yield cl
 
-    def generate_config_file(self):
+    def generate_config_file(self, classes=None):
         """generate default config file from Configurables"""
         lines = ["# Configuration file for %s." % self.name]
         lines.append('')
-        for cls in self._classes_with_config_traits():
-            lines.append(cls.class_config_section())
+        classes = self.classes if classes is None else classes
+        config_classes = list(self._classes_with_config_traits(classes))
+        for cls in config_classes:
+            lines.append(cls.class_config_section(config_classes))
         return '\n'.join(lines)
 
     def exit(self, exit_status=0):
diff --git a/traitlets/config/configurable.py b/traitlets/config/configurable.py
index d48c1e8..0d074bb 100644
--- a/traitlets/config/configurable.py
+++ b/traitlets/config/configurable.py
@@ -218,7 +218,7 @@ class Configurable(HasTraits):
         """
         assert inst is None or isinstance(inst, cls)
         final_help = []
-        base_classes = ','.join(p.__name__ for p in cls.__bases__)
+        base_classes = ', '.join(p.__name__ for p in cls.__bases__)
         final_help.append(u'%s(%s) options' % (cls.__name__, base_classes))
         final_help.append(len(final_help[0])*u'-')
         for k, v in sorted(cls.class_traits(config=True).items()):
@@ -284,8 +284,41 @@ class Configurable(HasTraits):
         print(cls.class_get_help(inst))
 
     @classmethod
-    def class_config_section(cls):
-        """Get the config class config section"""
+    def _defining_class(cls, trait, classes):
+        """Get the class that defines a trait
+
+        For reducing redundant help output in config files.
+        Returns the current class if:
+        - the trait is defined on this class, or
+        - the class where it is defined would not be in the config file
+
+        Parameters
+        ----------
+        trait: Trait
+            The trait to look for
+        classes: list
+            The list of other classes to consider for redundancy.
+            Will return `cls` even if it is not defined on `cls`
+            if the defining class is not in `classes`.
+        """
+        defining_cls = cls
+        for parent in cls.mro():
+            if issubclass(parent, Configurable) and \
+            parent in classes and \
+            parent.class_own_traits(config=True).get(trait.name, None) is trait:
+                defining_cls = parent
+        return defining_cls
+
+    @classmethod
+    def class_config_section(cls, classes=None):
+        """Get the config section for this class.
+        
+        Parameters
+        ----------
+        classes: list, optional
+            The list of other classes in the config file.
+            Used to reduce redundant information.
+        """
         def c(s):
             """return a commented, wrapped block."""
             s = '\n\n'.join(wrap_paragraphs(s, 78))
@@ -293,8 +326,12 @@ class Configurable(HasTraits):
             return '## ' + s.replace('\n', '\n#  ')
 
         # section header
-        breaker = '#' + '-'*78
-        parent_classes = ','.join(p.__name__ for p in cls.__bases__)
+        breaker = '#' + '-' * 78
+        parent_classes = ', '.join(
+            p.__name__ for p in cls.__bases__
+            if issubclass(p, Configurable)
+        )
+
         s = "# %s(%s) configuration" % (cls.__name__, parent_classes)
         lines = [breaker, s, breaker]
         # get the description trait
@@ -308,16 +345,29 @@ class Configurable(HasTraits):
             lines.append(c(desc))
             lines.append('')
 
-        for name, trait in sorted(cls.class_own_traits(config=True).items()):
-            lines.append(c(trait.help))
+        for name, trait in sorted(cls.class_traits(config=True).items()):
+            default_repr = trait.default_value_repr()
 
-            if 'Enum' in type(trait).__name__:
-                # include Enum choices
-                lines.append('#  Choices: %r' % (trait.values,))
+            if classes:
+                defining_class = cls._defining_class(trait, classes)
+            else:
+                defining_class = cls
+            if defining_class is cls:
+                # cls owns the trait, show full help
+                if trait.help:
+                    lines.append(c(trait.help))
+                if 'Enum' in type(trait).__name__:
+                    # include Enum choices
+                    lines.append('#  Choices: %r' % (trait.values,))
+                lines.append('#  Default: %s' % default_repr)
+            else:
+                # Trait appears multiple times and isn't defined here.
+                # Truncate help to first line + "See also Original.trait"
+                if trait.help:
+                    lines.append(c(trait.help.split('\n', 1)[0]))
+                lines.append('#  See also %s.%s' % (defining_class.__name__, name))
 
-            dvr = trait.default_value_repr()
-            lines.append(indent('#  Default: %s' % dvr, 4))
-            lines.append('#c.%s.%s = %s' % (cls.__name__, name, dvr))
+            lines.append('# c.%s.%s = %s' % (cls.__name__, name, default_repr))
             lines.append('')
         return '\n'.join(lines)
 
@@ -329,7 +379,7 @@ class Configurable(HasTraits):
         """
         lines = []
         classname = cls.__name__
-        for k, trait in sorted(cls.class_own_traits(config=True).items()):
+        for k, trait in sorted(cls.class_traits(config=True).items()):
             ttype = trait.__class__.__name__
 
             termline = classname + '.' + trait.name
@@ -352,7 +402,7 @@ class Configurable(HasTraits):
                     dvr = dvr[:61]+'...'
                 # Double up backslashes, so they get to the rendered docs
                 dvr = dvr.replace('\\n', '\\\\n')
-                lines.append('    Default: ``%s``' % dvr)
+                lines.append(indent('Default: ``%s``' % dvr, 4))
                 lines.append('')
 
             help = trait.help or 'No description'


Test patch:
diff --git a/traitlets/config/tests/test_application.py b/traitlets/config/tests/test_application.py
index 7f2757b..d0ec89b 100644
--- a/traitlets/config/tests/test_application.py
+++ b/traitlets/config/tests/test_application.py
@@ -33,13 +33,18 @@ from traitlets.config.application import (
 )
 
 from ipython_genutils.tempdir import TemporaryDirectory
-from traitlets.traitlets import (
+from traitlets import (
+    HasTraits,
     Bool, Unicode, Integer, List, Tuple, Set, Dict
 )
 
 class Foo(Configurable):
 
-    i = Integer(0, help="The integer i.").tag(config=True)
+    i = Integer(0, help="""
+    The integer i.
+
+    Details about i.
+    """).tag(config=True)
     j = Integer(1, help="The integer j.").tag(config=True)
     name = Unicode(u'Brian', help="First name.").tag(config=True)
     la = List([]).tag(config=True)
@@ -114,7 +119,7 @@ class TestApplication(TestCase):
         app = MyApp()
         self.assertEqual(app.name, u'myapp')
         self.assertEqual(app.running, False)
-        self.assertEqual(app.classes, [MyApp,Bar,Foo])
+        self.assertEqual(app.classes, [MyApp, Bar, Foo])
         self.assertEqual(app.config_file, u'')
 
     def test_mro_discovery(self):
@@ -406,17 +411,38 @@ class TestApplication(TestCase):
         assert 'The integer b.' in app.generate_config_file()
 
     def test_generate_config_file_classes_to_include(self):
-        class NoTraits(Foo, Bar):
+        class NotInConfig(HasTraits):
+            from_hidden = Unicode('x', help="""From hidden class
+            
+            Details about from_hidden.
+            """).tag(config=True)
+
+        class NoTraits(Foo, Bar, NotInConfig):
             pass
 
         app = MyApp()
         app.classes.append(NoTraits)
+
         conf_txt = app.generate_config_file()
+        print(conf_txt)
         self.assertIn('The integer b.', conf_txt)
-        self.assertIn('# Bar(Configurable)', conf_txt)
         self.assertIn('# Foo(Configurable)', conf_txt)
         self.assertNotIn('# Configurable', conf_txt)
-        self.assertIn('# NoTraits(Foo,Bar)', conf_txt)
+        self.assertIn('# NoTraits(Foo, Bar)', conf_txt)
+
+        # inherited traits, parent in class list:
+        self.assertIn('# c.NoTraits.i', conf_txt)
+        self.assertIn('# c.NoTraits.j', conf_txt)
+        self.assertIn('# c.NoTraits.n', conf_txt)
+        self.assertIn('#  See also Foo.j', conf_txt)
+        self.assertIn('#  See also Bar.b', conf_txt)
+        self.assertEqual(conf_txt.count('Details about i.'), 1)
+
+        # inherited traits, parent not in class list:
+        self.assertIn("# c.NoTraits.from_hidden", conf_txt)
+        self.assertNotIn('#  See also NotInConfig.', conf_txt)
+        self.assertEqual(conf_txt.count('Details about from_hidden.'), 1)
+        self.assertNotIn("NotInConfig", conf_txt)
 
     def test_multi_file(self):
         app = MyApp()