[trac384] handle review comments

actually just the first part of it

src/bin/bindctl/bindcmd.py

@@ -375,7 +375,15 @@ class BindCmdInterpreter(Cmd):
         if cmd.command == "help" or ("help" in cmd.params.keys()):
             self._handle_help(cmd)
         elif cmd.module == CONFIG_MODULE_NAME:
-            self.apply_config_cmd(cmd)
+            try:
+                self.apply_config_cmd(cmd)
+            except isc.cc.data.DataTypeError as dte:
+                print("Error: " + str(dte))
+            except isc.cc.data.DataNotFoundError as dnfe:
+                print("Error: " + str(dnfe))
+            except KeyError as ke:
+                print("Error: missing " + str(ke))
+                raise ke
         else:
             self.apply_cmd(cmd)
 
@@ -398,17 +406,22 @@ class BindCmdInterpreter(Cmd):
         print(CONST_BINDCTL_HELP)
         for k in self.modules.values():
             n = k.get_name()
-            if len(n) >= 8:
-                print("\t%s" % n)
+            if len(n) >= CONST_BINDCTL_HELP_INDENT_WIDTH:
+                print("    %s" % n)
                 print(textwrap.fill(k.get_desc(),
-                      initial_indent="\t\t",
-                      subsequent_indent="\t\t",
+                      initial_indent="            ",
+                      subsequent_indent="    " +
+                      " " * CONST_BINDCTL_HELP_INDENT_WIDTH,
                       width=70))
             else:
-                print(textwrap.fill("%s\t%s" % (k.get_name(), k.get_desc()),
-                      initial_indent="\t",
-                      subsequent_indent="\t\t",
-                      width=70))
+                print(textwrap.fill("%s%s%s" %
+                    (k.get_name(),
+                     " "*(CONST_BINDCTL_HELP_INDENT_WIDTH - len(k.get_name())),
+                     k.get_desc()),
+                    initial_indent="    ",
+                    subsequent_indent="    " +
+                    " " * CONST_BINDCTL_HELP_INDENT_WIDTH,
+                    width=70))
     
     def onecmd(self, line):
         if line == 'EOF' or line.lower() == "quit":
@@ -551,102 +564,94 @@ class BindCmdInterpreter(Cmd):
            Raises a KeyError if the command was not complete
         '''
         identifier = self.location
-        try:
-            if 'identifier' in cmd.params:
-                if not identifier.endswith("/"):
-                    identifier += "/"
-                if cmd.params['identifier'].startswith("/"):
-                    identifier = cmd.params['identifier']
-                else:
-                    if cmd.params['identifier'].startswith('['):
-                        identifier = identifier[:-1]
-                    identifier += cmd.params['identifier']
-
-                # Check if the module is known; for unknown modules
-                # we currently deny setting preferences, as we have
-                # no way yet to determine if they are ok.
-                module_name = identifier.split('/')[1]
-                if module_name != "" and (self.config_data is None or \
-                   not self.config_data.have_specification(module_name)):
-                    print("Error: Module '" + module_name + "' unknown or not running")
-                    return
+        if 'identifier' in cmd.params:
+            if not identifier.endswith("/"):
+                identifier += "/"
+            if cmd.params['identifier'].startswith("/"):
+                identifier = cmd.params['identifier']
+            else:
+                if cmd.params['identifier'].startswith('['):
+                    identifier = identifier[:-1]
+                identifier += cmd.params['identifier']
+
+            # Check if the module is known; for unknown modules
+            # we currently deny setting preferences, as we have
+            # no way yet to determine if they are ok.
+            module_name = identifier.split('/')[1]
+            if module_name != "" and (self.config_data is None or \
+               not self.config_data.have_specification(module_name)):
+                print("Error: Module '" + module_name + "' unknown or not running")
+                return
 
-            if cmd.command == "show":
-                # check if we have the 'all' argument
-                show_all = False
-                if 'argument' in cmd.params:
-                    if cmd.params['argument'] == 'all':
-                        show_all = True
-                    elif 'identifier' not in cmd.params:
-                        # no 'all', no identifier, assume this is the
-                        #identifier
-                        identifier += cmd.params['argument']
-                    else:
-                        print("Error: unknown argument " + cmd.params['argument'] + ", or multiple identifiers given")
-                        return
-                values = self.config_data.get_value_maps(identifier, show_all)
-                for value_map in values:
-                    line = value_map['name']
-                    if value_map['type'] in [ 'module', 'map' ]:
-                        line += "/"
-                    elif len(value_map) > 1 and value_map['type'] == 'list' \
-                         and (value_map['value'] != []):
-                        # do not print content of non-empty lists if
-                        # we have more data to show
-                        line += "/"
-                    else:
-                        line += "\t" + json.dumps(value_map['value'])
-                    line += "\t" + value_map['type']
-                    line += "\t"
-                    if value_map['default']:
-                        line += "(default)"
-                    if value_map['modified']:
-                        line += "(modified)"
-                    print(line)
-            elif cmd.command == "show_json":
-                if identifier == "":
-                    print("Need at least the module to show the configuration in JSON format")
+        if cmd.command == "show":
+            # check if we have the 'all' argument
+            show_all = False
+            if 'argument' in cmd.params:
+                if cmd.params['argument'] == 'all':
+                    show_all = True
+                elif 'identifier' not in cmd.params:
+                    # no 'all', no identifier, assume this is the
+                    #identifier
+                    identifier += cmd.params['argument']
                 else:
-                    data, default = self.config_data.get_value(identifier)
-                    print(json.dumps(data))
-            elif cmd.command == "add":
-                if 'value' in cmd.params:
-                    self.config_data.add_value(identifier, cmd.params['value'])
-                else:
-                    self.config_data.add_value(identifier)
-            elif cmd.command == "remove":
-                if 'value' in cmd.params:
-                    self.config_data.remove_value(identifier, cmd.params['value'])
-                else:
-                    self.config_data.remove_value(identifier, None)
-            elif cmd.command == "set":
-                if 'identifier' not in cmd.params:
-                    print("Error: missing identifier or value")
+                    print("Error: unknown argument " + cmd.params['argument'] + ", or multiple identifiers given")
+                    return
+            values = self.config_data.get_value_maps(identifier, show_all)
+            for value_map in values:
+                line = value_map['name']
+                if value_map['type'] in [ 'module', 'map' ]:
+                    line += "/"
+                elif len(value_map) > 1 and value_map['type'] == 'list' \
+                     and (value_map['value'] != []):
+                    # do not print content of non-empty lists if
+                    # we have more data to show
+                    line += "/"
                 else:
-                    parsed_value = None
-                    try:
-                        parsed_value = json.loads(cmd.params['value'])
-                    except Exception as exc:
-                        # ok could be an unquoted string, interpret as such
-                        parsed_value = cmd.params['value']
-                    self.config_data.set_value(identifier, parsed_value)
-            elif cmd.command == "unset":
-                self.config_data.unset(identifier)
-            elif cmd.command == "revert":
-                self.config_data.clear_local_changes()
-            elif cmd.command == "commit":
-                self.config_data.commit()
-            elif cmd.command == "diff":
-                print(self.config_data.get_local_changes());
-            elif cmd.command == "go":
-                self.go(identifier)
-        except isc.cc.data.DataTypeError as dte:
-            print("Error: " + str(dte))
-        except isc.cc.data.DataNotFoundError as dnfe:
-            print("Error: " + str(dnfe))
-        except KeyError as ke:
-            print("Error: missing " + str(ke))
-            raise ke
+                    line += "\t" + json.dumps(value_map['value'])
+                line += "\t" + value_map['type']
+                line += "\t"
+                if value_map['default']:
+                    line += "(default)"
+                if value_map['modified']:
+                    line += "(modified)"
+                print(line)
+        elif cmd.command == "show_json":
+            if identifier == "":
+                print("Need at least the module to show the configuration in JSON format")
+            else:
+                data, default = self.config_data.get_value(identifier)
+                print(json.dumps(data))
+        elif cmd.command == "add":
+            if 'value' in cmd.params:
+                self.config_data.add_value(identifier, cmd.params['value'])
+            else:
+                self.config_data.add_value(identifier)
+        elif cmd.command == "remove":
+            if 'value' in cmd.params:
+                self.config_data.remove_value(identifier, cmd.params['value'])
+            else:
+                self.config_data.remove_value(identifier, None)
+        elif cmd.command == "set":
+            if 'identifier' not in cmd.params:
+                print("Error: missing identifier or value")
+            else:
+                parsed_value = None
+                try:
+                    parsed_value = json.loads(cmd.params['value'])
+                except Exception as exc:
+                    # ok could be an unquoted string, interpret as such
+                    parsed_value = cmd.params['value']
+                self.config_data.set_value(identifier, parsed_value)
+        elif cmd.command == "unset":
+            self.config_data.unset(identifier)
+        elif cmd.command == "revert":
+            self.config_data.clear_local_changes()
+        elif cmd.command == "commit":
+            self.config_data.commit()
+        elif cmd.command == "diff":
+            print(self.config_data.get_local_changes());
+        elif cmd.command == "go":
+            self.go(identifier)
 
     def go(self, identifier):
         '''Handles the config go command, change the 'current' location

src/bin/bindctl/moduleinfo.py

@@ -32,6 +32,9 @@ MODULE_NODE_NAME = 'module'
 COMMAND_NODE_NAME = 'command'
 PARAM_NODE_NAME = 'param'
 
+# this is used to align the descriptions in help output
+CONST_BINDCTL_HELP_INDENT_WIDTH=12
+
 
 class ParamInfo:
     """One parameter of one command.
@@ -164,10 +167,10 @@ class CommandInfo:
         mandatory_infos = []
         for info in params.values():            
             if not info.is_optional:
-                print("\t%s" % info.get_name())
+                print("    %s" % info.get_name())
                 print(textwrap.fill(info.get_desc(),
-                      initial_indent="\t\t",
-                      subsequent_indent="\t\t",
+                      initial_indent="        ",
+                      subsequent_indent="        ",
                       width=50))
                 mandatory_infos.append(info)
 
@@ -176,10 +179,10 @@ class CommandInfo:
         if len(optional_infos) > 0:
             print("\nOptional parameters:")      
             for info in optional_infos:
-                print("\t%s" % info.get_name())
+                print("    %s" % info.get_name())
                 print(textwrap.fill(info.get_desc(),
-                      initial_indent="\t\t",
-                      subsequent_indent="\t\t",
+                      initial_indent="        ",
+                      subsequent_indent="        ",
                       width=50))
 
 
@@ -230,17 +233,22 @@ class ModuleInfo:
         print("Module ", self, "\nAvailable commands:")
         for k in self.commands.values():
             n = k.get_name()
-            if len(n) >= 8:
-                print("\t%s" % n)
+            if len(n) >= CONST_BINDCTL_HELP_INDENT_WIDTH:
+                print("    %s" % n)
                 print(textwrap.fill(k.get_desc(),
-                      initial_indent="\t\t",
-                      subsequent_indent="\t\t",
+                      initial_indent="            ",
+                      subsequent_indent="    " +
+                      " " * CONST_BINDCTL_HELP_INDENT_WIDTH,
                       width=70))
             else:
-                print(textwrap.fill("%s\t%s" % (k.get_name(), k.get_desc()),
-                      initial_indent="\t",
-                      subsequent_indent="\t\t",
-                      width=70))
+                print(textwrap.fill("%s%s%s" %
+                    (k.get_name(),
+                     " "*(CONST_BINDCTL_HELP_INDENT_WIDTH - len(k.get_name())),
+                     k.get_desc()),
+                    initial_indent="    ",
+                    subsequent_indent="    " +
+                    " " * CONST_BINDCTL_HELP_INDENT_WIDTH,
+                    width=70))
             
     def command_help(self, command):
         """Prints the help info for the command with the given name.

src/bin/bindctl/tests/bindctl_test.py

@@ -17,6 +17,8 @@
 import unittest
 import isc.cc.data
 import os
+from isc.config.config_data import ConfigData, MultiConfigData
+from isc.config.module_spec import ModuleSpec
 from bindctl import cmdparse
 from bindctl import bindcmd
 from bindctl.moduleinfo import *
@@ -238,11 +240,85 @@ class TestNameSequence(unittest.TestCase):
             assert self.random_names[i] == module_names[i+1]
             i = i + 1
 
-    def test_apply_cfg_command(self):
+# tine class to fake a UIModuleCCSession, but only the config data
+# parts for the next set of tests
+class FakeCCSession(MultiConfigData):
+    def __init__(self):
+        self._local_changes = {}
+        self._current_config = {}
+        self._specifications = {}
+        self.add_foo_spec()
+
+    def add_foo_spec(self):
+        spec = { "module_name": "foo",
+                 "config_data": [
+                 { "item_name": "an_int",
+                   "item_type": "integer",
+                   "item_optional": False,
+                   "item_default": 1
+                 },
+                 { "item_name": "a_list",
+                   "item_type": "list",
+                   "item_optional": False,
+                   "item_default": [],
+                   "list_item_spec":
+                   { "item_name": "a_string",
+                     "item_type": "string",
+                     "item_optional": False,
+                     "item_default": "bar"
+                   }
+                 }
+                 ]
+               }
+        self.set_specification(ModuleSpec(spec))
+    
+
+class TestConfigCommands(unittest.TestCase):
+    def setUp(self):
+        self.tool = bindcmd.BindCmdInterpreter()
+        mod_info = ModuleInfo(name = "foo")
+        self.tool.add_module_info(mod_info)
+        self.tool.config_data = FakeCCSession()
+        
+    def test_apply_cfg_command_int(self):
         self.tool.location = '/'
-        cmd = cmdparse.BindCmdParse("config set identifier=\"foo/bar\" value=\"5\"")
+
+        self.assertEqual((1, MultiConfigData.DEFAULT),
+                         self.tool.config_data.get_value("/foo/an_int"))
+
+        cmd = cmdparse.BindCmdParse("config set identifier=\"foo/an_int\" value=\"5\"")
         self.tool.apply_config_cmd(cmd)
+        self.assertEqual((5, MultiConfigData.LOCAL),
+                         self.tool.config_data.get_value("/foo/an_int"))
+
+        # this should raise a NotFoundError
+        cmd = cmdparse.BindCmdParse("config set identifier=\"foo/bar\" value=\"[]\"")
+        self.assertRaises(isc.cc.data.DataNotFoundError, self.tool.apply_config_cmd, cmd)
+
+        # this should raise a TypeError
+        cmd = cmdparse.BindCmdParse("config set identifier=\"foo/an_int\" value=\"[]\"")
+        self.assertRaises(isc.cc.data.DataTypeError, self.tool.apply_config_cmd, cmd)
+
+    def test_apply_cfg_command_list(self):
+        self.tool.location = '/'
+
+        self.assertEqual(([], MultiConfigData.DEFAULT),
+                         self.tool.config_data.get_value("/foo/a_list"))
+
+        cmd = cmdparse.BindCmdParse("config set identifier=\"foo/a_list\" value=[\"a\"]")
+        self.tool.apply_config_cmd(cmd)
+        self.assertEqual((["a"], MultiConfigData.LOCAL),
+                         self.tool.config_data.get_value("/foo/a_list"))
+
+        # this should raise a TypeError
+        cmd = cmdparse.BindCmdParse("config set identifier=\"foo/a_list\" value=\"a\"")
+        self.assertRaises(isc.cc.data.DataTypeError, self.tool.apply_config_cmd, cmd)
+        
+        cmd = cmdparse.BindCmdParse("config set identifier=\"foo/a_list\" value=[1]")
+        self.assertRaises(isc.cc.data.DataTypeError, self.tool.apply_config_cmd, cmd)
+
     
+
 class FakeBindCmdInterpreter(bindcmd.BindCmdInterpreter):
     def __init__(self):
         pass