launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/maascli-degenericize-register into lp:maas.

Commit message:
Make registration of "a" maascli module (in practice, the api module) a bit simpler by removing some genericity.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/maascli-degenericize-register/+merge/128183

The generic registration code was going a bit far: introspecting a module, recognizing and decoding a custom naming convention, intricate testing -- all to avoid listing explicitly which functions need to be exposed.  It would only have paid off with a large and constantly changing set of built-in commands, where we currently have only 4.

In a next step, we can move the built-in commands into their own subcommand, and then start making the module name optional.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/maascli-degenericize-register/+merge/128183
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/maascli-degenericize-register into lp:maas.

=== modified file 'src/maascli/__init__.py'
--- src/maascli/__init__.py	2012-10-05 04:36:10 +0000
+++ src/maascli/__init__.py	2012-10-05 09:44:22 +0000
@@ -19,10 +19,11 @@
 import sys
 
 from bzrlib import osutils
-from maascli.utils import (
-    parse_docstring,
-    safe_name,
+from maascli.api import (
+    register_api_commands,
+    register_cli_commands,
     )
+from maascli.utils import parse_docstring
 
 
 class ArgumentParser(argparse.ArgumentParser):
@@ -58,7 +59,8 @@
     parser = ArgumentParser(
         description=help_body, prog=argv[0],
         epilog="http://maas.ubuntu.com/";)
-    register(module, parser)
+    register_cli_commands(parser)
+    register_api_commands(parser)
 
     # Run, doing polite things with exceptions.
     try:
@@ -68,30 +70,3 @@
         raise SystemExit(1)
     except StandardError as error:
         parser.error("%s" % error)
-
-
-def register(module, parser, prefix="cmd_"):
-    """Register commands in `module` with the given argument parser.
-
-    This looks for callable objects named `cmd_*` by default, calls them with
-    a new subparser, and registers them as the default value for `execute` in
-    the namespace.
-
-    If the module also has a `register` function, this is also called, passing
-    in the module being scanned, and the parser given to this function.
-    """
-    # Register commands.
-    trim = slice(len(prefix), None)
-    commands = {
-        name[trim]: command for name, command in vars(module).items()
-        if name.startswith(prefix) and callable(command)
-        }
-    for name, command in commands.items():
-        help_title, help_body = parse_docstring(command)
-        command_parser = parser.subparsers.add_parser(
-            safe_name(name), help=help_title, description=help_body)
-        command_parser.set_defaults(execute=command(command_parser))
-    # Extra subparser registration.
-    register_module = getattr(module, "register", None)
-    if callable(register_module):
-        register_module(module, parser)

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2012-10-05 04:36:10 +0000
+++ src/maascli/api.py	2012-10-05 09:44:22 +0000
@@ -11,7 +11,8 @@
 
 __metaclass__ = type
 __all__ = [
-    "register",
+    "register_api_commands",
+    "register_cli_commands",
     ]
 
 from email.message import Message
@@ -377,8 +378,23 @@
         register_actions(profile, handler, handler_parser)
 
 
-def register(module, parser):
-    """Register profiles."""
+def register_cli_commands(parser):
+    """Register the CLI's meta-subcommands on `parser`."""
+    commands = {
+        'login': cmd_login,
+        'logout': cmd_logout,
+        'list': cmd_list,
+        'refresh': cmd_refresh,
+    }
+    for name, command in commands.items():
+        help_title, help_body = parse_docstring(command)
+        command_parser = parser.subparsers.add_parser(
+            safe_name(name), help=help_title, description=help_body)
+        command_parser.set_defaults(execute=command(command_parser))
+
+
+def register_api_commands(parser):
+    """Register all profiles as subcommands on `parser`."""
     with ProfileConfig.open() as config:
         for profile_name in config:
             profile = config[profile_name]

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2012-10-05 04:36:10 +0000
+++ src/maascli/tests/test_api.py	2012-10-05 09:44:22 +0000
@@ -20,8 +20,12 @@
 
 from apiclient.creds import convert_tuple_to_string
 import httplib2
-from maascli import api
+from maascli import (
+    api,
+    ArgumentParser,
+    )
 from maascli.command import CommandError
+from maascli.config import ProfileConfig
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
 from mock import sentinel
@@ -33,6 +37,68 @@
     )
 
 
+class FakeConfig(dict):
+    """Fake `ProfileConfig`.  A dict that's also a context manager."""
+    def __enter__(self, *args, **kwargs):
+        return self
+
+    def __exit__(self, *args, **kwargs):
+        pass
+
+
+class TestRegisterAPICommands(TestCase):
+    """Tests for `register_api_commands`."""
+
+    def make_profile(self):
+        """Fake a profile."""
+        profile = factory.make_name('profile')
+        url = 'http://%s.example.com/' % profile
+        fake_open = self.patch(ProfileConfig, 'open')
+        fake_open.return_value = FakeConfig({
+            profile: {
+                'name': profile,
+                'url': url,
+                'description': {
+                    'handlers': [{
+                        'name': factory.make_name('handler'),
+                        'doc': "Short\n\nLong",
+                        'params': [],
+                        'actions': [{
+                            'name': factory.make_name('action'),
+                            'doc': "Doc\n\nstring",
+                        }],
+                    }],
+                },
+            },
+        })
+        return profile
+
+    def test_registers_subparsers(self):
+        profile = self.make_profile()
+        parser = ArgumentParser()
+        self.assertIsNone(parser._subparsers)
+        api.register_api_commands(parser)
+        self.assertIsNotNone(parser._subparsers)
+        self.assertIsNotNone(parser.subparsers.choices[profile])
+
+
+class TestRegisterCLICommands(TestCase):
+    """Tests for `register_cli_commands`."""
+
+    def test_registers_subparsers(self):
+        parser = ArgumentParser()
+        self.assertIsNone(parser._subparsers)
+        api.register_cli_commands(parser)
+        self.assertIsNotNone(parser._subparsers)
+
+    def test_subparsers_have_appropriate_execute_defaults(self):
+        parser = ArgumentParser()
+        api.register_cli_commands(parser)
+        self.assertIsInstance(
+            parser.subparsers.choices['login'].get_default('execute'),
+            api.cmd_login)
+
+
 class TestFunctions(TestCase):
     """Test for miscellaneous functions in `maascli.api`."""
 

=== modified file 'src/maascli/tests/test_init.py'
--- src/maascli/tests/test_init.py	2012-09-16 21:00:54 +0000
+++ src/maascli/tests/test_init.py	2012-10-05 09:44:22 +0000
@@ -12,14 +12,8 @@
 __metaclass__ = type
 __all__ = []
 
-import new
-
-from maascli import (
-    ArgumentParser,
-    register,
-    )
+from maascli import ArgumentParser
 from maastesting.testcase import TestCase
-from mock import sentinel
 
 
 class TestArgumentParser(TestCase):
@@ -42,72 +36,3 @@
         # The subparsers property, once populated, always returns the same
         # object.
         self.assertIs(subparsers, parser.subparsers)
-
-
-class TestRegister(TestCase):
-    """Tests for `maascli.register`."""
-
-    def test_empty(self):
-        module = new.module(b"%s.test" % __name__)
-        parser = ArgumentParser()
-        register(module, parser)
-        # No subparsers were registered.
-        self.assertIsNone(parser._subparsers)
-
-    def test_command(self):
-        module = new.module(b"%s.test" % __name__)
-        cmd = self.patch(module, "cmd_one")
-        cmd.return_value = sentinel.execute
-        parser = ArgumentParser()
-        register(module, parser)
-        # Subparsers were registered.
-        self.assertIsNotNone(parser._subparsers)
-        # The command was called once with a subparser called "one".
-        subparser_one = parser.subparsers.choices["one"]
-        cmd.assert_called_once_with(subparser_one)
-        # The subparser has an appropriate execute default.
-        self.assertIs(
-            sentinel.execute,
-            subparser_one.get_default("execute"))
-
-    def test_commands(self):
-        module = new.module(b"%s.test" % __name__)
-        cmd_one = self.patch(module, "cmd_one")
-        cmd_one.return_value = sentinel.x_one
-        cmd_two = self.patch(module, "cmd_two")
-        cmd_two.return_value = sentinel.x_two
-        parser = ArgumentParser()
-        register(module, parser)
-        # The commands were called with appropriate subparsers.
-        subparser_one = parser.subparsers.choices["one"]
-        cmd_one.assert_called_once_with(subparser_one)
-        subparser_two = parser.subparsers.choices["two"]
-        cmd_two.assert_called_once_with(subparser_two)
-        # The subparsers have appropriate execute defaults.
-        self.assertIs(sentinel.x_one, subparser_one.get_default("execute"))
-        self.assertIs(sentinel.x_two, subparser_two.get_default("execute"))
-
-    def test_register(self):
-        module = new.module(b"%s.test" % __name__)
-        module_register = self.patch(module, "register")
-        parser = ArgumentParser()
-        register(module, parser)
-        # No subparsers were registered; calling module.register does not
-        # imply that this happens.
-        self.assertIsNone(parser._subparsers)
-        # The command was called once with a subparser called "one".
-        module_register.assert_called_once_with(module, parser)
-
-    def test_command_and_register(self):
-        module = new.module(b"%s.test" % __name__)
-        module_register = self.patch(module, "register")
-        cmd = self.patch(module, "cmd_one")
-        parser = ArgumentParser()
-        register(module, parser)
-        # Subparsers were registered because a command was found.
-        self.assertIsNotNone(parser._subparsers)
-        # The command was called once with a subparser called "one".
-        module_register.assert_called_once_with(module, parser)
-        # The command was called once with a subparser called "one".
-        cmd.assert_called_once_with(
-            parser.subparsers.choices["one"])