commands: Extend use of exit helper to parser errors

This moves all the bad arugment handling into our class and makes sure to mark
all the helpers as internal.

mopidy/utils/command.py

@@ -4,22 +4,17 @@ import os
 import sys
 
 
-class CommandError(Exception):
+class _ParserError(Exception):
-    def __init__(self, message, usage=None):
+    pass
-        self.message = message
-        self.usage = usage
-
-    def __str__(self):
-        return '%s\n\nerror: %s' % (self.usage, self.message)
-
-
-class ArgumentParser(argparse.ArgumentParser):
-    def error(self, message):
-        raise CommandError(message)
 
 
 class _HelpError(Exception):
-    """Internal exception used to trigger help code path."""
+    pass
+
+
+class _ArgumentParser(argparse.ArgumentParser):
+    def error(self, message):
+        raise _ParserError(message)
 
 
 class _HelpAction(argparse.Action):
@@ -43,7 +38,7 @@ class Command(object):
 
     def _build(self):
         actions = []
-        parser = ArgumentParser(add_help=False)
+        parser = _ArgumentParser(add_help=False)
         parser.register('action', 'help', _HelpAction)
 
         for args, kwargs in self._arguments:
@@ -62,8 +57,9 @@ class Command(object):
     def set_defaults(self, **kwargs):
         self._defaults.update(kwargs)
 
-    def exit(self, return_code):
+    def exit(self, status_code=0, message=None, usage=None):
-        sys.exit(return_code)
+        print '\n\n'.join(m for m in (usage, message) if m.strip)
+        sys.exit(status_code)
 
     def format_usage(self, prog=None):
         actions = self._build()[1]
@@ -124,8 +120,7 @@ class Command(object):
             return self._parse(
                 args, argparse.Namespace(), self._defaults.copy(), prog)
         except _HelpError:
-            print self.format_help(prog)
+            self.exit(0, self.format_help(prog))
-            self.exit(0)
 
     def _parse(self, args, namespace, defaults, prog):
         defaults.update(self._defaults)
@@ -133,9 +128,8 @@ class Command(object):
 
         try:
             result = parser.parse_args(args, namespace)
-        except CommandError as e:
+        except _ParserError as e:
-            e.usage = self._usage(actions, prog)
+            self.exit(1, e.message, self._usage(actions, prog))
-            raise
 
         if not result._args:
             for attr, value in defaults.items():
@@ -147,8 +141,8 @@ class Command(object):
 
         child = result._args.pop(0)
         if child not in self._children:
-            raise CommandError('unrecognized command: %s' % child,
+            usage = self._usage(actions, prog)
-                               usage=self._usage(actions, prog))
+            self.exit(1, 'unrecognized command: %s' % child, usage)
 
         return self._children[child]._parse(
             result._args, result, defaults, ' '.join([prog, child]))

tests/utils/command_test.py

@@ -8,6 +8,14 @@ from mopidy.utils import command
 
 
 class CommandParsingTest(unittest.TestCase):
+    def setUp(self):
+        self.exit_patcher = mock.patch.object(command.Command, 'exit')
+        self.exit_mock = self.exit_patcher.start()
+        self.exit_mock.side_effect = SystemExit
+
+    def tearDown(self):
+        self.exit_patcher.stop()
+
     def test_command_parsing_returns_namespace(self):
         cmd = command.Command()
         self.assertIsInstance(cmd.parse([]), argparse.Namespace)
@@ -17,14 +25,14 @@ class CommandParsingTest(unittest.TestCase):
         result = cmd.parse([])
         self.assertFalse(hasattr(result, '_args'))
 
-    def test_unknown_options_raises_error(self):
+    def test_unknown_options_bails(self):
         cmd = command.Command()
-        with self.assertRaises(command.CommandError):
+        with self.assertRaises(SystemExit):
             cmd.parse(['--foobar'])
 
-    def test_invalid_sub_command_raises_error(self):
+    def test_invalid_sub_command_bails(self):
         cmd = command.Command()
-        with self.assertRaises(command.CommandError):
+        with self.assertRaises(SystemExit):
             cmd.parse(['foo'])
 
     def test_command_arguments(self):
@@ -94,12 +102,12 @@ class CommandParsingTest(unittest.TestCase):
         cmd = command.Command()
         cmd.add_argument('--bar', type=int)
 
-        with self.assertRaises(command.CommandError) as cm:
+        with self.assertRaises(SystemExit):
             cmd.parse(['--bar', b'zero'], prog='foo')
 
-        self.assertEqual(cm.exception.message,
+        self.exit_mock.assert_called_once_with(
-                         "argument --bar: invalid int value: 'zero'")
+            1, "argument --bar: invalid int value: 'zero'",
-        self.assertEqual(cm.exception.usage, 'usage: foo [--bar BAR]')
+            'usage: foo [--bar BAR]')
 
     @mock.patch('sys.argv')
     def test_command_error_usage_prog(self, argv_mock):
@@ -108,33 +116,37 @@ class CommandParsingTest(unittest.TestCase):
         cmd = command.Command()
         cmd.add_argument('--bar', required=True)
 
-        with self.assertRaises(command.CommandError) as cm:
+        with self.assertRaises(SystemExit):
             cmd.parse([])
-        self.assertEqual(cm.exception.usage, 'usage: foo --bar BAR')
+        self.exit_mock.assert_called_once_with(
+            mock.ANY, mock.ANY, 'usage: foo --bar BAR')
 
-        with self.assertRaises(command.CommandError) as cm:
+        self.exit_mock.reset_mock()
+        with self.assertRaises(SystemExit):
             cmd.parse([], prog='baz')
-        self.assertEqual(cm.exception.usage, 'usage: baz --bar BAR')
+
+        self.exit_mock.assert_called_once_with(
+            mock.ANY, mock.ANY, 'usage: baz --bar BAR')
 
     def test_missing_required(self):
         cmd = command.Command()
         cmd.add_argument('--bar', required=True)
 
-        with self.assertRaises(command.CommandError) as cm:
+        with self.assertRaises(SystemExit):
             cmd.parse([], prog='foo')
 
-        self.assertEqual(cm.exception.message, 'argument --bar is required')
+        self.exit_mock.assert_called_once_with(
-        self.assertEqual(cm.exception.usage, 'usage: foo --bar BAR')
+            1, 'argument --bar is required', 'usage: foo --bar BAR')
 
     def test_missing_positionals(self):
         cmd = command.Command()
         cmd.add_argument('bar')
 
-        with self.assertRaises(command.CommandError) as cm:
+        with self.assertRaises(SystemExit):
             cmd.parse([], prog='foo')
 
-        self.assertEqual(cm.exception.message, 'too few arguments')
+        self.exit_mock.assert_called_once_with(
-        self.assertEqual(cm.exception.usage, 'usage: foo bar')
+            1, 'too few arguments', 'usage: foo bar')
 
     def test_missing_positionals_subcommand(self):
         child = command.Command()
@@ -143,31 +155,30 @@ class CommandParsingTest(unittest.TestCase):
         cmd = command.Command()
         cmd.add_child('bar', child)
 
-        with self.assertRaises(command.CommandError) as cm:
+        with self.assertRaises(SystemExit):
             cmd.parse(['bar'], prog='foo')
 
-        self.assertEqual(cm.exception.message, 'too few arguments')
+        self.exit_mock.assert_called_once_with(
-        self.assertEqual(cm.exception.usage, 'usage: foo bar baz')
+            1, 'too few arguments', 'usage: foo bar baz')
 
     def test_unknown_command(self):
         cmd = command.Command()
 
-        with self.assertRaises(command.CommandError) as cm:
+        with self.assertRaises(SystemExit):
             cmd.parse(['--help'], prog='foo')
 
-        self.assertEqual(
+        self.exit_mock.assert_called_once_with(
-            cm.exception.message, 'unrecognized arguments: --help')
+            1, 'unrecognized arguments: --help', 'usage: foo')
-        self.assertEqual(cm.exception.usage, 'usage: foo')
 
     def test_invalid_subcommand(self):
         cmd = command.Command()
         cmd.add_child('baz', command.Command())
 
-        with self.assertRaises(command.CommandError) as cm:
+        with self.assertRaises(SystemExit):
             cmd.parse(['bar'], prog='foo')
 
-        self.assertEqual(cm.exception.message, 'unrecognized command: bar')
+        self.exit_mock.assert_called_once_with(
-        self.assertEqual(cm.exception.usage, 'usage: foo')
+            1, 'unrecognized command: bar', 'usage: foo')
 
     def test_set_defaults(self):
         cmd = command.Command()
@@ -201,11 +212,12 @@ class CommandParsingTest(unittest.TestCase):
         cmd = command.Command()
         cmd.add_argument('-h', action='help')
         cmd.format_help = mock.Mock()
-        cmd.exit = mock.Mock()
 
-        cmd.parse(['-h'])
+        with self.assertRaises(SystemExit):
+            cmd.parse(['-h'])
+
         cmd.format_help.assert_called_once_with(mock.ANY)
-        cmd.exit.assert_called_once_with(0)
+        self.exit_mock.assert_called_once_with(0, cmd.format_help.return_value)
 
 
 class UsageTest(unittest.TestCase):
@@ -435,17 +447,6 @@ class HelpTest(unittest.TestCase):
         self.assertEqual(expected, cmd.format_help('foo').strip())
 
 
-class CommandErrorTest(unittest.TestCase):
-    def test_args_get_stored(self):
-        error = command.CommandError('message', usage='usage: foo')
-        self.assertEqual(error.message, 'message')
-        self.assertEqual(error.usage, 'usage: foo')
-
-    def test_str_command_error(self):
-        error = command.CommandError('message', usage='usage: foo')
-        self.assertEqual(str(error), 'usage: foo\n\nerror: message')
-
-
 class RunTest(unittest.TestCase):
     def test_default_implmentation_raises_error(self):
         with self.assertRaises(NotImplementedError):