On Thu, May 07, 2020 at 05:41:57PM +0200, Andrea Bolognani wrote:
> Right now we catch and report properly only those exceptions that
> are raised by the Application.run() method: basically, we work
> under the assumption that nothing before that point can possibly
> fail.
>
> That's of course unrealistic: even now, creating an Inventory or
> Projects object can raise an exception, in which case we simply
> display a stack trace instead of a reasonable error message.
>
> This commit introduces a CommandLine class which takes over
> command line handling from the Application class, and moves all
> exception handling to the main() method so that, short of
> something going horribly wrong while parsing the command line,
> we will always manage to hide stack traces from the user.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> guests/lcitool | 56 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/guests/lcitool b/guests/lcitool
> index ab3b95f..ff58829 100755
> --- a/guests/lcitool
> +++ b/guests/lcitool
> @@ -390,15 +390,9 @@ class Projects:
> return self._packages[project]
>
>
> -class Application:
> +class CommandLine:
>
> def __init__(self):
> - self._config = Config()
> - self._inventory = Inventory()
> - self._projects = Projects()
> -
> - self._native_arch = Util.get_native_arch()
> -
> self._parser = argparse.ArgumentParser(
> conflict_handler="resolve",
> description="libvirt CI guest management tool",
> @@ -444,14 +438,14 @@ class Application:
>
> installparser = subparsers.add_parser(
> "install", help="perform unattended host installation")
> - installparser.set_defaults(func=self._action_install)
> + installparser.set_defaults(action="install")
please, keep the usage of the 'func' attribute:
.set_defaults(func=Application._action_install)
...
>
> - def run(self):
> - args = self._parser.parse_args()
> - if args.debug:
> - args.func(args)
> - else:
> - try:
> - args.func(args)
> - except Exception as err:
> - sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
> - sys.exit(1)
> + def run(self, args):
> + getattr(self, "_action_" + args.action)(args)
Okay, ^this is obfuscation:
1) the action attribute should not be misused for your own metadata, it defines
a small set of actions to take when the option was seen on the cmdline [1]
- for arbitrary actions, instantiate argparse.Action instead (also to be
seen found at [1]) - you don't want to do this either in this case,
instead, respect the meaning of the 'func' attribute and use it in one of the
following ways:
# this one is still a bit obfuscated, but conforms to the encapsulation
# principles
self.__getattribute__(args.func.__name__)(args)
OR
# this IMO breaks the OOP principles and encapsulation, but at the same time is
# very much readable
args.func(self, args)
2) goes against the mantra you mentioned recently in your review:
"Explicit is better than implicit" (where implicit doesn't even apply here :))
[1] https://docs.python.org/3/library/argparse.html#action
With that addressed:
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
--
Erik Skultety
