Repository: dssg/argcmdr

Issue:
local call argument
`Local` commands should receive their `local` OS-lookup object as an optional invocation argument (as with `args` and `parser`).

For example:

```python
class PrintDir(Local):

    def prepare(self, args, parser, local):
        return local['ls']
```

Discussion:

Patch:
diff --git a/README.rst b/README.rst
index 6e81099..2ec2304 100644
--- a/README.rst
+++ b/README.rst
@@ -172,7 +172,7 @@ In fact, our above, trivial example could be accomplished easily with direct exe
 
 ``local``, bound to the ``Local`` base class, is a dictionary which caches path look-ups for system executables.
 
-This could, however, still be cleaner. For this reason, the ``Local`` command features a parallel invocation interface, ``prepare([args, parser, ...])``::
+This could, however, still be cleaner. For this reason, the ``Local`` command features a parallel invocation interface, ``prepare([args, parser, local, ...])``::
 
     class Main(Local):
         """list directory contents"""
@@ -336,7 +336,7 @@ Command invocation signature
 
 Note that in our last trivial examples of listing directory contents, we made our script dependent upon the ``ls`` command in the operating environment. ``argcmdr`` will not, by default, print tracebacks, and it will colorize unhandled exceptions; however, we might prefer to print a far friendlier error message.
 
-One easy way of printing friendly error messages is to make use of ``argparse.ArgumentParser.error()``. As we've seen, ``Command`` invocation, via either ``__call__`` or ``prepare``, may accept zero arguments, or it may require the parsed arguments ``argparse.Namespace``. Moreover, it may require a second argument, and receive the argument parser::
+One easy way of printing friendly error messages is to make use of ``argparse.ArgumentParser.error()``. As we've seen, ``Command`` invocation, via either ``__call__`` or ``prepare``, may accept zero arguments, or it may require the parsed arguments ``argparse.Namespace``. Moreover, it may require a second argument to receive the argument parser, and a third argument to receive the ``local`` dictionary::
 
     class Main(Local):
         """list directory contents"""
@@ -348,10 +348,10 @@ One easy way of printing friendly error messages is to make use of ``argparse.Ar
                 nargs=argparse.REMAINDER,
             )
 
-        def prepare(self, args, parser):
+        def prepare(self, args, parser, local):
             try:
-                local_exec = self.local['ls']
-            except self.local.CommandNotFound:
+                local_exec = local['ls']
+            except local.CommandNotFound:
                 parser.error('command not available')
 
             yield local_exec[args.remainder]
@@ -619,7 +619,7 @@ Let's define the executable file ``git`` with no particular purpose whatsoever::
         """stash the changes in a dirty working directory away"""
 
         def __call__(self, args):
-            self['save'](args)
+            self['save'].delegate()
 
         class Save(Command):
             """save your local modifications to a new stash"""
@@ -635,8 +635,7 @@ Let's define the executable file ``git`` with no particular purpose whatsoever::
                 )
 
             def __call__(self, args):
-                interactive = getattr(args, 'interactive', False)
-                print("stash save", f"(interactive: {interactive})")
+                print("stash save", f"(interactive: {args.interactive})")
 
         class List(Command):
             """list the stashes that you currently have"""
@@ -669,15 +668,14 @@ Decorator-manufactured commands are no less capable than those derived from clas
         """stash the changes in a dirty working directory away"""
 
         def __call__(self, args):
-            self['save'](args)
+            self['save'].delegate()
 
         @cmd('-p', '--patch', dest='interactive', action='store_true', default=False,
              help="interactively select hunks from the diff between HEAD "
                   "and the working tree to be stashed")
         def save(args):
             """save your local modifications to a new stash"""
-            interactive = getattr(args, 'interactive', False)
-            print("stash save", f"(interactive: {interactive})")
+            print("stash save", f"(interactive: {args.interactive})")
 
         @cmd
         def list():
@@ -692,12 +690,11 @@ Say, however, that we needed to invert the factoring of ``save`` logic between t
     class Stash(Command):
         """stash the changes in a dirty working directory away"""
 
-        def perform_save(self, args):
-            interactive = getattr(args, 'interactive', False)
+        def perform_save(self, interactive=False):
             print("stash save", f"(interactive: {interactive})")
 
-        def __call__(self, args):
-            self.perform_save(args)
+        def __call__(self):
+            self.perform_save()
 
         @cmd('-p', '--patch', dest='interactive', action='store_true', default=False,
              help="interactively select hunks from the diff between HEAD "
@@ -705,7 +702,7 @@ Say, however, that we needed to invert the factoring of ``save`` logic between t
         @cmd(binding=True)
         def save(self, args):
             """save your local modifications to a new stash"""
-            self[-1].perform_save(args)
+            self[-1].perform_save(args.interactive)
 
         @cmd
         def list():
@@ -724,19 +721,18 @@ To improve on the above, we may instead decorate our command function with ``cmd
     class Stash(Command):
         """stash the changes in a dirty working directory away"""
 
-        def perform_save(self, args):
-            interactive = getattr(args, 'interactive', False)
+        def perform_save(self, interactive=False):
             print("stash save", f"(interactive: {interactive})")
 
-        def __call__(self, args):
-            self.perform_save(args)
+        def __call__(self):
+            self.perform_save()
 
         @cmdmethod('-p', '--patch', dest='interactive', action='store_true', default=False,
                    help="interactively select hunks from the diff between HEAD "
                         "and the working tree to be stashed")
         def save(self, args):
             """save your local modifications to a new stash"""
-            self.perform_save(args)
+            self.perform_save(args.interactive)
 
 The ``cmdmethod`` decorator – as well as the complementary ``localmethod`` decorator – alter the binding of the decorated function such that it receives the instance of its parent command – not itself – upon invocation. Much cleaner.
 
@@ -775,6 +771,47 @@ Because command look-ups are relative to the current command, ``Command`` also o
 
     self.root['stash', 'save']
 
+Command delegation
+~~~~~~~~~~~~~~~~~~
+
+As you've seen above, command instance subscription enables access to ancestor and descendent commands from the command hierarchy.
+
+And, simple ``Command`` instances may be executed directly via ``__call__``. However, above, we instead invoked the ``delegate`` method. Why?
+
+* ``__call__`` must be invoked as defined – including its argument signature – which may or may not include ``args`` and/or ``parser`` (and which may change during development)
+* The ``args`` and ``parser`` in the scope of the delegating command – (generally the command actually selected by user argumentation) – reflect the arguments defined for that command, *not* those of the delegated command.
+
+For ``Local`` command instances, the situation, without ``delegate``, is worse:
+
+* To generate system commands (rather than executing them immediately), we must know to target ``prepare`` rather than ``__call__``
+
+For example, above, our ``Stash`` command might look like the following without ``delegate``::
+
+    class Stash(Command):
+        """stash the changes in a dirty working directory away"""
+
+        def __call__(self, args):
+            self['save'](args)
+
+        class Save(Command):
+            """save your local modifications to a new stash"""
+
+            def __init__(self, parser):
+                parser.add_argument(
+                    '-p', '--patch',
+                    dest='interactive',
+                    action='store_true',
+                    default=False,
+                    help="interactively select hunks from the diff between HEAD "
+                         "and the working tree to be stashed",
+                )
+
+            def __call__(self, args):
+                interactive = getattr(args, 'interactive', False)
+                print("stash save", f"(interactive: {interactive})")
+
+Note, in ``Stash.__call__``, the passing through of ``args``; and, in ``Stash.Save.__call__``, the use of ``getattr``. With ``delegate``, neither is required.
+
 The management file
 -------------------
 
diff --git a/src/argcmdr.py b/src/argcmdr.py
index 9fad761..595ba36 100644
--- a/src/argcmdr.py
+++ b/src/argcmdr.py
@@ -2,6 +2,7 @@ import argcomplete
 import argparse
 import collections
 import collections.abc
+import copy
 import enum
 import functools
 import importlib
@@ -14,7 +15,7 @@ import sys
 
 import plumbum
 import plumbum.commands
-from descriptors import classproperty
+from descriptors import cachedproperty, classproperty
 from plumbum import colors
 
 
@@ -68,7 +69,7 @@ def main(command_class,
         argcomplete.autocomplete(parser)
         parser.parse_args(argv, args)
         command = args.__command__
-        command.call(args)
+        command._call_()
     except Exception as exc:
         if args is None or getattr(args, 'traceback', True):
             raise
@@ -247,9 +248,11 @@ class Command:
     formatter_class = argparse.HelpFormatter
 
     def __init__(self, parser):
+        self.args = None
+        self._parser_ = None
+
         self.__children__ = None
         self.__parents__ = None
-        self._args = None
 
     def __call__(self, args):
         args.__parser__.print_usage()
@@ -286,15 +289,69 @@ class Command:
         if self.__parents__:
             return self.__parents__[-1]
 
+    def get_args(self):
+        args = self.__dict__.get('args')
+
+        if args is None:
+            raise RuntimeError('parsed argument namespace not available at this stage')
+
+        return args
+
     @property
     def args(self):
-        if getattr(self, '_args', None) is None:
-            raise RuntimeError('parsed argument namespace not available at this stage')
-        else:
-            return self._args
+        args = self.get_args()
 
-    def call(self, args, target_name='__call__'):
-        call_args = (args, args.__parser__)
+        if args.__command__ is not self:
+            # The property is here made consistent with the argumentation of
+            # the delegate() interface; and, methods relying on this property
+            # are thereby enabled regardless of whether their associated
+            # command was invoked directly or via delegation.
+            #
+            # Note: It is arguable that this is unnecessary in the case that
+            # the requesting command is an ancestor of the CLI-invoked
+            # command -- in this case, all of the requesting command's
+            # arguments should be filled, and there's little need.
+            #
+            # This condition could be tested here with the expression:
+            #
+            #     self (not) in args.__command__.__parents__
+            #
+            # (and in which case, for consistency, delegate() should perhaps be
+            # modified to also pass non-delegate args to the ancestor command).
+            #
+            # However, more than just the CLI arguments, delegate_args further
+            # populates the command's own sub-parser. And so, for now, for
+            # consistency and relative simplicity, all commands will *always*
+            # get their "own" args and parser, via delegate_args whenever it is
+            # not the CLI-invoked command.
+            #
+            return self.delegate_args
+
+        return args
+
+    @args.setter
+    def args(self, namespace):
+        self.__dict__['args'] = namespace
+
+    @cachedproperty
+    def delegate_args(self):
+        args = copy.copy(self.get_args())
+        args.__parser__ = self._parser_
+
+        for action in self._parser_._actions:
+            args.__dict__.setdefault(action.dest, action.default)
+
+        for default in self._parser_._defaults.items():
+            args.__dict__.setdefault(*default)
+
+        return args
+
+    def _call_(self, *additional, base_args=None, target_name='__call__'):
+        if base_args is None:
+            nspace = self.get_args()
+            base_args = (nspace, nspace.__parser__)
+
+        call_args = base_args + additional
         call_arg_count = len(call_args)
 
         target_callable = getattr(self, target_name)
@@ -312,6 +369,18 @@ class Command:
 
         return target_callable(*call_args[:param_count])
 
+    def get_user_signature(self):
+        return ((), {})
+
+    def _user_call(self):
+        (call_args, call_kwargs) = self.get_user_signature()
+        return self._call_(*call_args, **call_kwargs)
+
+    def delegate(self):
+        (args, kwargs) = self.get_user_signature()
+        base_args = (self.delegate_args, self.delegate_args.__parser__)
+        return self._call_(*args, base_args=base_args, **kwargs)
+
     @classproperty
     def name(cls):
         return cls.__name__.lower()
@@ -363,7 +432,8 @@ class Command:
         command = cls(parser)
         command.__parents__ = parents
         command.__children__ = {}
-        command._args = namespace
+        command.args = namespace
+        command._parser_ = parser
 
         if chain is not None:
             chain[cls.name] = command
@@ -530,8 +600,11 @@ class Local(Command):
             formulation = ' '.join(map(str, command.formulate()))
             print('>', colors['#5FAF5F'] | formulation)
 
+    def get_user_signature(self):
+        return ((self.local,), {'target_name': 'prepare'})
+
     def __call__(self, args):
-        commands = self.call(args, 'prepare')
+        commands = self._user_call()
 
         if commands is None:
             return


Test patch:
diff --git a/test/test_argcmdr.py b/test/test_argcmdr.py
index a0ff085..2d02e9b 100644
--- a/test/test_argcmdr.py
+++ b/test/test_argcmdr.py
@@ -2,6 +2,7 @@ import argparse
 import io
 import os
 import pdb
+import re
 import subprocess
 import types
 import unittest
@@ -17,6 +18,8 @@ from argcmdr import *
 from argcmdr import CommandMethod, GeneratedCommand, exhaust_iterable
 
 
+ANSI_ESCAPE = re.compile(r'\x1B\[[0-?]*[ -/]*[@-~]')
+
 TEST_DIR = os.path.dirname(__file__)
 DATA_DIR = os.path.join(TEST_DIR, 'data')
 
@@ -34,7 +37,7 @@ class TryCommandTestCase(unittest.TestCase):
         (self.parser, args) = command_cls.get_parser()
         self.parser.parse_args([], args)
         command = args.__command__
-        command.call(args)
+        command._call_()
 
 
 class TryMainTestCase(unittest.TestCase):
@@ -114,6 +117,237 @@ class TestCommandGetItem(unittest.TestCase):
             self.root[0]
 
 
+class TestCommandDelegation(unittest.TestCase):
+
+    def test_delegation_to_child(test):
+        @cmd('--no-eat', action='store_false', default=True, dest='should_eat')
+        @cmd(root=True, binding=True)
+        def root(self, args):
+            test.assertTrue(args.should_eat)
+            test.assertFalse(hasattr(args, 'this_food'))
+            test.assertIs(args, self.get_args())
+
+            # This would fail!
+            # (forcing Child to handle missing 'this_food')
+            # self['child'](args)
+            #
+            # This won't ;)
+            self['child'].delegate()
+
+        @root.register
+        @cmd('--food', default='snacks', dest='this_food')
+        @cmd(binding=True)
+        def child(self, args):
+            # target command's args populated regardless
+            test.assertTrue(args.should_eat)
+
+            # delegation should populate this command's defaults
+            test.assertEqual(args.this_food, 'snacks')
+
+            # ...in args property as well
+            test.assertEqual(self.args.this_food, 'snacks')
+
+            # self.args is self.delegate_args
+            test.assertIs(self.args, self.delegate_args)
+
+            # ...is args
+            test.assertIs(args, self.args)
+
+            # ...not the "real" args
+            test.assertIsNot(args, self.get_args())
+
+            # ...only the target gets those:
+            test.assertIs(self.root.args, self.get_args())
+
+        (parser, args) = root.get_parser()
+        parser.parse_args([], args)
+        args.__command__._call_()
+
+    def test_delegation_to_root(test):
+        @cmd('--no-eat', action='store_false', default=True, dest='should_eat')
+        @cmd(root=True, binding=True)
+        def root(self, args, parser):
+            # root *also* has child's args, because child was called
+            test.assertTrue(args.should_eat)
+            test.assertEqual(args.this_food, 'snacks')
+
+            # root gets delegate_args -- even though it doesn't *strictly* need it
+            #
+            # (child's args are superset of root's, so this could be omitted in this case,
+            # perhaps as an optimization.)
+            #
+            # however, this way, root gets access to *its* subparser, (on the off-chance that
+            # that matters). and, we're just consistent and straight-forward about it.
+            test.assertIs(args, self.args)
+            test.assertIs(self.args, self.delegate_args)
+
+            test.assertIs(parser, args.__parser__)
+            test.assertIs(parser, self._parser_)
+
+        @root.register
+        @cmd('--food', default='snacks', dest='this_food')
+        @cmd(binding=True)
+        def child(self, args):
+            test.assertTrue(args.should_eat)
+            test.assertEqual(args.this_food, 'snacks')
+
+            # self.args is NOT self.delegate_args
+            test.assertIs(args, self.get_args())
+            test.assertIs(args, self.args)
+            test.assertIsNot(self.args, self.delegate_args)
+
+            self.root.delegate()
+
+        (parser, args) = root.get_parser()
+        parser.parse_args(['child'], args)
+        args.__command__._call_()
+
+    def test_delegation_to_sibling(test):
+        @cmd('--no-eat', action='store_false', default=True, dest='should_eat', root=True)
+        def root():
+            raise AssertionError("I should not be invoked")
+
+        @root.register
+        @cmd('--candy', default='jelly beans', dest='this_candy', binding=True)
+        def left(self, args, parser):
+            test.assertTrue(args.should_eat)
+            test.assertEqual(args.this_food, 'snacks')
+
+            # delegation should populate this command's defaults
+            test.assertEqual(args.this_candy, 'jelly beans')
+
+            # self.args is self.delegate_args
+            test.assertIs(self.args, self.delegate_args)
+
+            # ...is args
+            test.assertIs(args, self.args)
+
+            # ...not the "real" args
+            test.assertIsNot(args, self.get_args())
+
+        @root.register
+        @cmd('--food', default='snacks', dest='this_food', binding=True)
+        def right(self, args):
+            test.assertTrue(args.should_eat)
+            test.assertEqual(args.this_food, 'snacks')
+
+            test.assertFalse(hasattr(args, 'this_candy'))
+
+            # self.args is NOT self.delegate_args
+            test.assertIs(args, self.get_args())
+            test.assertIs(args, self.args)
+            test.assertIsNot(self.args, self.delegate_args)
+
+            self[-1, 'left'].delegate()
+
+        (parser, args) = root.get_parser()
+        parser.parse_args(['right'], args)
+        args.__command__._call_()
+
+    def test_arbitrary_access(test):
+        @cmd('--no-eat', action='store_false', default=True, dest='should_eat', root=True)
+        def root():
+            raise AssertionError("I should not be invoked")
+
+        @root.register
+        class Left(Command):
+
+            def __init__(self, parser):
+                parser.add_argument(
+                    '--candy',
+                    default='jelly beans',
+                    dest='this_candy',
+                )
+
+            def __call__(self):
+                raise AssertionError("I should not be invoked")
+
+            def method(self):
+                test.assertTrue(self.args.should_eat)
+                test.assertEqual(self.args.this_food, 'snacks')
+
+                # delegation should populate this command's defaults
+                test.assertEqual(self.args.this_candy, 'jelly beans')
+
+                # self.args is self.delegate_args
+                test.assertIs(self.args, self.delegate_args)
+
+                # ...not the "real" args
+                test.assertIsNot(self.args, self.get_args())
+
+        @root.register
+        @cmd('--food', default='snacks', dest='this_food', binding=True)
+        def right(self, args):
+            test.assertTrue(args.should_eat)
+            test.assertEqual(args.this_food, 'snacks')
+
+            test.assertFalse(hasattr(args, 'this_candy'))
+
+            # self.args is NOT self.delegate_args
+            test.assertIs(args, self.get_args())
+            test.assertIs(args, self.args)
+            test.assertIsNot(self.args, self.delegate_args)
+
+            self[-1, 'left'].method()
+
+        (parser, args) = root.get_parser()
+        parser.parse_args(['right'], args)
+        args.__command__._call_()
+
+    def test_parser_defaults(test):
+        """delegate_args respects ArgumentParser.set_defaults"""
+        class Root(RootCommand):
+
+            def __init__(self, parser):
+                parser.add_argument(
+                    '--no-eat',
+                    action='store_false',
+                    default=True,
+                    dest='should_eat',
+                )
+
+                parser.set_defaults(root='so cool')
+
+            def __call__(self, args):
+                test.assertTrue(args.should_eat)
+                test.assertEqual(args.root, 'so cool')
+                test.assertFalse(hasattr(args, 'this_food'))
+                test.assertIs(args, self.get_args())
+
+                self['child'].delegate()
+
+        @Root.register
+        class Child(Command):
+
+            def __init__(self, parser):
+                parser.add_argument(
+                    '--food',
+                    default='snacks',
+                    dest='this_food',
+                )
+
+                parser.set_defaults(child='even cooler')
+
+            def __call__(self, args):
+                test.assertTrue(args.should_eat)
+                test.assertEqual(args.root, 'so cool')
+
+                # delegation should populate this command's defaults
+                test.assertEqual(args.this_food, 'snacks')
+                test.assertEqual(args.child, 'even cooler')
+
+                # self.args is self.delegate_args
+                test.assertIs(args, self.args)
+                test.assertIs(self.args, self.delegate_args)
+
+                # ...not the "real" args
+                test.assertIsNot(args, self.get_args())
+
+        (parser, args) = Root.get_parser()
+        parser.parse_args([], args)
+        args.__command__._call_()
+
+
 class TestCommandRoot(unittest.TestCase):
 
     @classmethod
@@ -233,10 +467,23 @@ class TestPrepareCallSignature(TryCommandTestCase):
 
         self.try_command(Deep)
 
+    def test_local(self):
+        class DeepLocal(Local):
+
+            def prepare(self_, args, parser, local):
+                self.assertIsInstance(self_, DeepLocal)
+                self.assertIsInstance(args, argparse.Namespace)
+                self.assertIs(args.__command__, self_)
+                self.assertIs(args.__parser__, self.parser)
+                self.assertIs(parser, self.parser)
+                self.assertIs(local, self_.local)
+
+        self.try_command(DeepLocal)
+
     def test_fancy(self):
         class Fancy(Local):
 
-            def prepare(self_, args, parser, lang='en'):
+            def prepare(self_, args, parser, local, lang='en'):
                 self.assertIsInstance(self_, Fancy)
                 self.assertIsInstance(args, argparse.Namespace)
                 self.assertIs(args.__command__, self_)
@@ -257,7 +504,7 @@ class TestPrepareCallSignature(TryCommandTestCase):
     def test_bad(self):
         class Bad(Local):
 
-            def prepare(self_, args, parser, lang):
+            def prepare(self_, args, parser, local, lang):
                 self.fail("how did this happen?")
 
         with self.assertRaises(TypeError) as context:
@@ -271,7 +518,7 @@ class TestPrepareCallSignature(TryCommandTestCase):
     def test_tricky(self):
         class Tricky(Local):
 
-            def prepare(self_, args, parser=None, lang='en'):
+            def prepare(self_, args, parser=None, local=None, lang='en'):
                 self.assertIsInstance(self_, Tricky)
                 self.assertIsInstance(args, argparse.Namespace)
                 self.assertIs(args.__command__, self_)
@@ -336,9 +583,6 @@ class TestSendCommandResult(TryCommandTestCase):
             def prepare(self_, args):
                 args.execute_commands = False
 
-                # don't clutter test output
-                args.show_commands = False
-
                 (code, std, err) = yield self_.local['which']['python']
 
                 self.assertIsNone(code)
@@ -351,7 +595,14 @@ class TestSendCommandResult(TryCommandTestCase):
                 self.assertIsNone(std)
                 self.assertIsNone(err)
 
-        self.try_command(SmartCommand)
+        with mock.patch('sys.stdout', new=io.StringIO()) as output:
+            self.try_command(SmartCommand)
+
+        plain_output = ANSI_ESCAPE.sub('', output.getvalue())
+        self.assertEqual(plain_output,
+            '> /usr/bin/which python\n'
+            '> /usr/bin/which TOTAL-FAKE\n'
+        )
 
     def test_non_gen(self):
         command = mock.Mock(spec=Local.local['which']['python'])