Refactor exit_code implementation

[kotfu]

What is the rationale behind the code in cmdloop() which calls sys.exit(self.exit_code) if the exit_code attribute is set?

I have lots of questions about the implementation, but I'll save them until I understand what we were trying to solve with this code.

[tleonhardt]

The rationale behind the exit_code stuff was #358 - the intent was to add the capability for cmd2 to return a non-zero exit code when exiting the cmdloop.

[tleonhardt]

For the implementation if I can remember correctly, I think I tried to go for the simplest possible solution #507.

From your question, can I conclude that I screwed something up? 😀

[kmvanbrunt]

@kotfu I've been following your progress in cmdsh with interest since I've considered making a lightweight cmd2 at times.

In it, you have a Result class:

@attr.s(frozen=True)
class Result():
    """The result of running a command
    The default result has an exit_code of 0 (indicating no errors or success), and stop
    is False (meaning the cmdloop() continues)
    """
    exit_code = attr.ib(default=0, validator=attr.validators.instance_of(int))
    stop = attr.ib(default=False, validator=attr.validators.instance_of(bool))

Are you thinking maybe cmd2 should bundle our exit code in a similar class?

[kotfu]

I wouldn't say you screwed anything up 👌. But the implementation seems 'unpythonic' to me. More than 15 years ago, Guido provided the pattern for writing command line applications that return exit codes: https://www.artima.com/weblogs/viewpost.jsp?thread=4829

Instead of throwing SystemExit exceptions (which happens when you call sys.exit()), he suggests you return exit code values all the way up to some code that looks like this:

if __name__ == "__main__":
    sys.exit(main())

Basically, he's advocating a "return a status code" method of gracefully exiting the program, instead of a "finally catch exceptions and re-raise them" method of gracefully exiting the program. We have implemented the latter. The following example demonstrates two problems with the current implementation:

class MyExceptionApp(cmd2.Cmd()):
    do_bailout_success(args):
        self.exit_code = 0
        return True

    do_bailout_error(args):
        self.exit_code = 1
        return True

    do_side_effects(args):
        # problem 1
        # even though this method returns false, the cmd loop exits, it's a non-obvious side effect
        self.exit_code = 0
        return False

app = MyExceptionApp()
# problem 2, if I don't catch SystemExit, I can't do anything after the command loop
try:
    app.cmdloop()
catch SystemExit as exc:
    print("application completed")
    sys.exit(exc.code)

With no support from cmd2, it's very easy to return exit codes to the shell. Every do_command method an already request the command loop stop, by returning a True value. We already have code which gracefully exits the cmdloop() function when this happens. To add the ability to return exit codes to the shell, you could:

class MyReturnApp(cmd2.Cmd()):
    do_bailout_success(args):
        self.shell_exit_code = 0
        return True

    do_bailout_error(args):
        self.shell_exit_code = 1
        return True

def main():
    app = MyReturnApp()
    app.cmdloop()
    print("application completed")
    return app.shell_exit_code

if __name__ == "__main__":
    sys.exit(main())

I believe cmd2 doesn't need any built-in support for exit codes. But if we feel we must, I'd recommend we put the following code at the bottom of the cmdloop():

return self.exit_code

That would yield the following modification to the MyReturnApp() example above:

def main():
    app = MyReturnApp()
    shell_exit_code = app.cmdloop()
    print("application completed")
    return shell_exit_code

These examples show a very simple bit of code for what happens after cmdloop() returns, and so it might not seem like a big difference. But if you have more complex code, it's all inside a catch or finally block.

So, if you are still reading this lengthy post, here are the things I don't like about the current implementation:

1. It feels unpythonic to me
2. The non-obvious side effect of setting the exit_code attribute
3. The catch or finally incantation required to do cleanup code in the thing that instantiates a cmd2 based app

[kmvanbrunt]

@kotfu Those are good points. Exiting the program in cmdloop() does seem like a bad idea.

[kotfu]

@kmvanbrunt I wanted to explore three things with the cmdsh experiment:

1. If I wanted to sacrifice drop-in compatibility with cmd from the standard library, but still have familiarity, what might I do differently. The Result class was one thing I would change.
2. Is it possible to encapsulate all the parsing/quoting bits inside of something like a "Personality" class. If you wanted a strictly bash compatible syntax, you could write a "BashPersonality" class. That class would be composed of a bash-flavored StatementParser, completer, whatever else we need to encapsulate. If you wanted a python style syntax with different quoting rules, you might write a "PythonPersonality" class. etc. I'm not far enough along to see how this would actually work.
3. Can the approach described in objective 2 allow to you have a library that can either be super lightweight, or as robust as you want just by switching personalities? In essence, is it possible to have all the "weight" be in the personality, not in the core library?

The Result class from cmdsh would be difficult to implement in cmd2 because it would break every single do_command() method ever written. Maybe we could subclass bool and make it work. However, back to objective 1 above, the subclassing of string we already have feels brittle and forced. We do it to maintain backwards compatibility with the standard library. If you gave that up, can the code be cleaner?