Add '_cmd_cls' special keyword for passing a RunningCommand subclass

[gsakkis]

Sometimes it's necessary to customize how a command is run in a way that:
(a) it is not supported by any existing special keyword arguments and
(b) it doesn't make sense to add a new special keyword to sh for it because it is very specific to the particular use case.

This PR introduces a new special keyword, _cmd_cls (feel free to pick a better name), that allows the client to pass a custom RunningCommand subclass. This opens up new possibilities that aren't currently possible (at least not without monkeypatching sh.RunningCommand).

[amoffat]

Interesting. What kinds of things are you doing/anticipating that would be accomplished with a custom class? I'm not opposed to the idea, but I am curious

[gsakkis]

My original use case was logging each command before it runs as well as its stdout, stderr and exit code (if not zero) after it runs, something like this:

import sh

class LoggingRunningCommand(sh.RunningCommand):

    def __init__(self, cmd, call_args, stdin, stdout, stderr):
        print '$ {}'.format(' '.join(cmd))
        try:
            super(LoggingRunningCommand, self).__init__(cmd, call_args, stdin,
                                                        stdout, stderr)
        except sh.ErrorReturnCode:
            pass

        self._log('STDOUT', self.stdout)
        self._log('STDERR', self.stderr)
        if self.exit_code:
            print '*** ABORTED (exit_code={}) ***'.format(self.exit_code)
            exit(self.exit_code)

    @staticmethod
    def _log(label, text):
        if text.strip():
            print '>>> START {label}\n{text}\n<<< END {label}\n'.format(**locals())

if __name__ == '__main__':
    sh.ls(__file__, 'foo', _cmd_cls=LoggingRunningCommand)

Output:

$ /bin/ls test_sh.py foo
>>> START STDOUT
test_sh.py

<<< END STDOUT

>>> START STDERR
/bin/ls: cannot access foo: No such file or directory

<<< END STDERR

*** ABORTED (exit_code=2) ***

But I can imagine more complex things one could do, like storing in a list all commands/outputs/exit codes that are run within a session, to be used for auditing, replaying, etc.

[amoffat]

It's an interesting idea. I'll tell you my thought process... currently the existing RunningCommand class isn't abstracted well enough to be used well as a base class. It seems to work for your example, but for some of the more complicated stuff you mentioned, it would need to have a clear API for derived classes to use. That API isn't there currently.

By accepting this work, I would be adding something that isn't fully able to be used reliably (because of the incomplete/unstable base-class API), but I'm not yet convinced that the need is there to warrant building it out. So I don't want to add something that's incomplete, nor do I want to add complexity for something that is unlikely to be used often.

I know this doesn't help your current situation. sh does have some decent logging happening, which can be activated by logging.basicConfig(level=logging.INFO) from your main script. Is this logging verbose enough for you? I wonder if we can elaborate on the logging to make it more useful.

[gsakkis]

I see what you're saying and it's definitely an advanced feature, requiring one to dive into the sh source to subclass RunningCommand. Thus I would suggest not documenting the _cmd_cls keyword if it was to be added now, but there's no harm in making it possible if the long term goal is to enentually cleanup and abstract RunningCommand so that it is reusable and extensible.

As for logging, it's maybe useful for debugging purposes but not really suitable for end-user feedback. For one thing, if there is no error it doesn't print the stdout and/or stderr, and the command itself is truncated:

INFO:sh.command:<Command '/bin/ls test_sh.py t...(31 more)' call_args {'bg': False, 'in_bu...(516 more)>: starting process

If there is an error, it prints out the full command, stdout and stderr but also prints the full traceback which I'd rather not show:

INFO:sh.command:<Command '/bin/ls test_sh.py t...(35 more)' call_args {'bg': False, 'in_bu...(516 more)>: starting process
Traceback (most recent call last):
  File "test_sh.py", line 29, in <module>
    sh.ls(__file__, __file__, __file__, __file__, 'foo')
  File "/home/gsakkis/projects/sh/sh.py", line 1025, in __call__
    return running_cmd_cls(cmd, call_args, stdin, stdout, stderr)
  File "/home/gsakkis/projects/sh/sh.py", line 486, in __init__
    self.wait()
  File "/home/gsakkis/projects/sh/sh.py", line 500, in wait
    self.handle_command_exit_code(exit_code)
  File "/home/gsakkis/projects/sh/sh.py", line 516, in handle_command_exit_code
    raise exc(self.ran, self.process.stdout, self.process.stderr)
sh.ErrorReturnCode_2: 

  RAN: '/bin/ls test_sh.py test_sh.py test_sh.py test_sh.py foo'

  STDOUT:
test_sh.py  test_sh.py  test_sh.py  test_sh.py


  STDERR:
/bin/ls: cannot access foo: No such file or directory

These choices make sense from logging point of view (sh shouldn't try to print arbitrarily long stdout/stderr strings) but the caller should be able to customize what happens before and after a command runs.

[marascio]

FWIW, to get reasonable logging I had to do the same thing, except I just monkey patched the sh module, which left me feeling dirty. Accepting the command class via keyword is one way, but I would prefer to be able to specify it globally as well thus not having to litter up my hundreds of sh invocations.

[amoffat]

Closing this with similar thoughts as my previous comment