Allow command hooks to make changes

This changes the signature of two private methods, but also expects
the new-ish Command.before() and Command.after() methods to now return
values.  We ensure that the previous behavior does not change if
nothing is returned explicitly.

Change-Id: I1cc0958dbce0c2516891df64f60d4bfd84c2e1a0

4 files changed, 225 insertions, 7 deletions

diff --git a/cliff/command.py b/cliff/command.py
index 1c6ac48..0d4fc73 100644
--- a/cliff/command.py
+++ b/cliff/command.py
@@ -133,9 +133,9 @@ class Command(object):
 
         Return the value returned by :meth:`take_action` or 0.
         """
-        self._run_before_hooks(parsed_args)
+        parsed_args = self._run_before_hooks(parsed_args)
         return_code = self.take_action(parsed_args) or 0
-        self._run_after_hooks(parsed_args, return_code)
+        return_code = self._run_after_hooks(parsed_args, return_code)
         return return_code
 
     def _run_before_hooks(self, parsed_args):
@@ -149,7 +149,12 @@ class Command(object):
         hook processing behavior.
         """
         for hook in self._hooks:
-            hook.obj.before(parsed_args)
+            ret = hook.obj.before(parsed_args)
+            # If the return is None do not change parsed_args, otherwise
+            # set up to pass it to the next hook
+            if ret is not None:
+                parsed_args = ret
+        return parsed_args
 
     def _run_after_hooks(self, parsed_args, return_code):
         """Calls after() method of the hooks.
@@ -162,7 +167,12 @@ class Command(object):
         hook processing behavior.
         """
         for hook in self._hooks:
-            hook.obj.after(parsed_args, return_code)
+            ret = hook.obj.after(parsed_args, return_code)
+            # If the return is None do not change return_code, otherwise
+            # set up to pass it to the next hook
+            if ret is not None:
+                return_code = ret
+        return return_code
 
 
 class _SmartHelpFormatter(_argparse.HelpFormatter):
diff --git a/cliff/display.py b/cliff/display.py
index 84c23ee..82b71d6 100644
--- a/cliff/display.py
+++ b/cliff/display.py
@@ -110,10 +110,11 @@ class DisplayCommandBase(command.Command):
         return columns_to_include, selector
 
     def run(self, parsed_args):
-        self._run_before_hooks(parsed_args)
+        parsed_args = self._run_before_hooks(parsed_args)
         self.formatter = self._formatter_plugins[parsed_args.formatter].obj
         column_names, data = self.take_action(parsed_args)
-        self._run_after_hooks(parsed_args, (column_names, data))
+        column_names, data = self._run_after_hooks(parsed_args,
+                                                   (column_names, data))
         self.produce_output(parsed_args, column_names, data)
         return 0
 
diff --git a/cliff/hooks.py b/cliff/hooks.py
index 9af627c..eee99a1 100644
--- a/cliff/hooks.py
+++ b/cliff/hooks.py
@@ -51,6 +51,7 @@ class CommandHook(object):
         :param parsed_args: The arguments to the command.
         :paramtype parsed_args: argparse.Namespace
         """
+        return parsed_args
 
     @abc.abstractmethod
     def after(self, parsed_args, return_code):
@@ -63,3 +64,4 @@ class CommandHook(object):
         :param return_code: The value returned from take_action().
         :paramtype return_code: int
         """
+        return return_code
diff --git a/cliff/tests/test_command_hooks.py b/cliff/tests/test_command_hooks.py
index 589f067..b8c7c4d 100644
--- a/cliff/tests/test_command_hooks.py
+++ b/cliff/tests/test_command_hooks.py
@@ -97,6 +97,75 @@ class TestHook(hooks.CommandHook):
         self._after_called = True
 
 
+class TestChangeHook(hooks.CommandHook):
+
+    _before_called = False
+    _after_called = False
+
+    def get_parser(self, parser):
+        parser.add_argument('--added-by-hook')
+        return parser
+
+    def get_epilog(self):
+        return 'hook epilog'
+
+    def before(self, parsed_args):
+        self._before_called = True
+        parsed_args.added_by_hook = 'othervalue'
+        parsed_args.added_by_before = True
+        return parsed_args
+
+    def after(self, parsed_args, return_code):
+        self._after_called = True
+        return 24
+
+
+class TestDisplayChangeHook(hooks.CommandHook):
+
+    _before_called = False
+    _after_called = False
+
+    def get_parser(self, parser):
+        parser.add_argument('--added-by-hook')
+        return parser
+
+    def get_epilog(self):
+        return 'hook epilog'
+
+    def before(self, parsed_args):
+        self._before_called = True
+        parsed_args.added_by_hook = 'othervalue'
+        parsed_args.added_by_before = True
+        return parsed_args
+
+    def after(self, parsed_args, return_code):
+        self._after_called = True
+        return (('Name',), ('othervalue',))
+
+
+class TestListerChangeHook(hooks.CommandHook):
+
+    _before_called = False
+    _after_called = False
+
+    def get_parser(self, parser):
+        parser.add_argument('--added-by-hook')
+        return parser
+
+    def get_epilog(self):
+        return 'hook epilog'
+
+    def before(self, parsed_args):
+        self._before_called = True
+        parsed_args.added_by_hook = 'othervalue'
+        parsed_args.added_by_before = True
+        return parsed_args
+
+    def after(self, parsed_args, return_code):
+        self._after_called = True
+        return (('Name',), [('othervalue',)])
+
+
 class TestCommandLoadHooks(base.TestBase):
 
     def test_no_app_or_name(self):
@@ -147,8 +216,54 @@ class TestHooks(base.TestBase):
 
     def test_after(self):
         self.assertFalse(self.hook._after_called)
-        self.cmd.run(None)
+        result = self.cmd.run(None)
         self.assertTrue(self.hook._after_called)
+        self.assertEqual(result, 42)
+
+
+class TestChangeHooks(base.TestBase):
+
+    def setUp(self):
+        super(TestChangeHooks, self).setUp()
+        self.app = make_app()
+        self.cmd = TestCommand(self.app, None, cmd_name='test')
+        self.hook = TestChangeHook(self.cmd)
+        self.mgr = extension.ExtensionManager.make_test_instance(
+            [extension.Extension(
+                'parser-hook',
+                None,
+                None,
+                self.hook)],
+        )
+        # Replace the auto-loaded hooks with our explicitly created
+        # manager.
+        self.cmd._hooks = self.mgr
+
+    def test_get_parser(self):
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        self.assertEqual(results.added_by_hook, 'value')
+
+    def test_get_epilog(self):
+        results = self.cmd.get_epilog()
+        self.assertIn('hook epilog', results)
+
+    def test_before(self):
+        self.assertFalse(self.hook._before_called)
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        self.cmd.run(results)
+        self.assertTrue(self.hook._before_called)
+        self.assertEqual(results.added_by_hook, 'othervalue')
+        self.assertTrue(results.added_by_before)
+
+    def test_after(self):
+        self.assertFalse(self.hook._after_called)
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        result = self.cmd.run(results)
+        self.assertTrue(self.hook._after_called)
+        self.assertEqual(result, 24)
 
 
 class TestShowOneHooks(base.TestBase):
@@ -193,6 +308,51 @@ class TestShowOneHooks(base.TestBase):
         self.assertTrue(self.hook._after_called)
 
 
+class TestShowOneChangeHooks(base.TestBase):
+
+    def setUp(self):
+        super(TestShowOneChangeHooks, self).setUp()
+        self.app = make_app()
+        self.cmd = TestShowCommand(self.app, None, cmd_name='test')
+        self.hook = TestDisplayChangeHook(self.cmd)
+        self.mgr = extension.ExtensionManager.make_test_instance(
+            [extension.Extension(
+                'parser-hook',
+                None,
+                None,
+                self.hook)],
+        )
+        # Replace the auto-loaded hooks with our explicitly created
+        # manager.
+        self.cmd._hooks = self.mgr
+
+    def test_get_parser(self):
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        self.assertEqual(results.added_by_hook, 'value')
+
+    def test_get_epilog(self):
+        results = self.cmd.get_epilog()
+        self.assertIn('hook epilog', results)
+
+    def test_before(self):
+        self.assertFalse(self.hook._before_called)
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        self.cmd.run(results)
+        self.assertTrue(self.hook._before_called)
+        self.assertEqual(results.added_by_hook, 'othervalue')
+        self.assertTrue(results.added_by_before)
+
+    def test_after(self):
+        self.assertFalse(self.hook._after_called)
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        result = self.cmd.run(results)
+        self.assertTrue(self.hook._after_called)
+        self.assertEqual(result, 0)
+
+
 class TestListerHooks(base.TestBase):
 
     def setUp(self):
@@ -233,3 +393,48 @@ class TestListerHooks(base.TestBase):
         results = parser.parse_args(['--added-by-hook', 'value'])
         self.cmd.run(results)
         self.assertTrue(self.hook._after_called)
+
+
+class TestListerChangeHooks(base.TestBase):
+
+    def setUp(self):
+        super(TestListerChangeHooks, self).setUp()
+        self.app = make_app()
+        self.cmd = TestListerCommand(self.app, None, cmd_name='test')
+        self.hook = TestListerChangeHook(self.cmd)
+        self.mgr = extension.ExtensionManager.make_test_instance(
+            [extension.Extension(
+                'parser-hook',
+                None,
+                None,
+                self.hook)],
+        )
+        # Replace the auto-loaded hooks with our explicitly created
+        # manager.
+        self.cmd._hooks = self.mgr
+
+    def test_get_parser(self):
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        self.assertEqual(results.added_by_hook, 'value')
+
+    def test_get_epilog(self):
+        results = self.cmd.get_epilog()
+        self.assertIn('hook epilog', results)
+
+    def test_before(self):
+        self.assertFalse(self.hook._before_called)
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        self.cmd.run(results)
+        self.assertTrue(self.hook._before_called)
+        self.assertEqual(results.added_by_hook, 'othervalue')
+        self.assertTrue(results.added_by_before)
+
+    def test_after(self):
+        self.assertFalse(self.hook._after_called)
+        parser = self.cmd.get_parser('test')
+        results = parser.parse_args(['--added-by-hook', 'value'])
+        result = self.cmd.run(results)
+        self.assertTrue(self.hook._after_called)
+        self.assertEqual(result, 0)