Code review comment for lp:~frankban/launchpad/lpsetup-initial

Thank you for the review Gary. I agree with your suggestions and I will reply only to your doubts concerning BaseSubcommand.

> I find the interaction between ArgumentParser (the actions attribute) and
> ActionsBasedSubCommand to not be obvious. I thought about it, and the main
> thing I found confusing was BaseSubCommand. ArgumentParser, AFAICT, has code
> specifically for ActionsBasedSubCommand, so it seems like it is specific to
> that kind of SubCommand. When would one actually use BaseSubCommand? The
> fact that we are not using the BaseSubCommand directly reinforces this
> feeling. Maybe a bit of elaboration would explain the value of having a
> separate BaseSubCommand, and not merging it directly with
> ActionsBasedSubCommand. In your cover letter you say it is for extensibility,
> but that does not convince me.

ArgumentParser ignores the concept of *actions* as intended in ActionBasedSubcommand.
The register_subcommand method just:
- instantiates a sub command object
- calls subcommand.add_arguments to collect subcommand args
- stores a reference to subcommand.main that can be used later

This interface is implemented by BaseSubCommand. ActionBasedSubcommand is a specialization only useful when a sub command is subdivided in steps (internal functions) and we want to expose them in the ui.

I think the misunderstanding comes from the different meaning of the word *action* in argparse and ActionBasedSubcommand.
In argparse an action is an object containing info about a single argument (argparse.Action). Actions are stored in argparse.ArgumentParser._actions and returned by argparse.ArgumentParser.add_argument(...). In my ArgumentParser subclass I store those actions in a "public" attribute because I need to reuse them later: I preferred to populate a new attribute over using a "private" one (like in setuplxc).

I will agree if you suggest to change the name of ArgumentParser.actions to avoid confusion.