Launchpad itself

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

lpsetup-initial
Merge into devel

Revision history for this message

Gary Poster (gary) wrote on 2012-02-27:

Continuing on from where I left off...

I suggest deleting update_launchpad_lxc for now, though I see why you have it. Up to you.

You have your file nicely divided in sections by function. If you were going to keep everything in this one file, I'd ask for comment lines delineating the sections--in particular, I had to think to realize that we were in the arg parsing section when I saw handle_user. Since you are going to divide this up into files anyway, I won't worry about it now.

You are not using os.path.expanduser in handle_directories for a reason: you are using the home directory of the user that was passed in, not the current home directory. A comment explaining this might be helpful, or might not.

The ArgumentParser might be improved if it showed in the docstring how users would use the registered subcommands. I expect that kind of thing might fall out of the division into separate files.

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.

When I run the command's help, I get the following output:

"""
$ ./lpsetup.py -h
usage: lpsetup.py [-h] {install,update,lxc-install} ...

Create and update Launchpad development and testing environments.

optional arguments:
-h, --help show this help message and exit

subcommands:
valid subcommands

  {install,update,lxc-install}
                        -h, --help show subcommand help and exit
    install Install the Launchpad environment.
    update Update the Launchpad environment to latest version.
    lxc-install Install the Launchpad environment inside an LXC.
"""
- I find the text "valid subcommands {install,update,lxc-install}" to be unnecessary and confusing.
- I find the placement of "-h, --help..." to be confusing too. Perhaps put that text, written more explicitly, where the "calid subcommands" text is? "Each subcommand accepts --h or --help to describe it."? Also, this reminds me, and probably other people who will use lpsetup, of bzr. They will want to do the following:

$ lpsetup help
(this could be equivalent to lpsetup -h)

$ lpsetup help install
(this could be equivalent to lpsetup install -h)
- The first (usage) line looks like you ought to be able to write "lpsetup -h install". You can, but it shows the top-level help--not what I'd expect. I think this usage line is confusing. Ideally, I think, the usage would be "lpsetup {help,install,update,lxc-install} ..." or something like that.

That's it. Nice progress! Thank you.

Continuing on from where I left off...

I suggest deleting update_launchpad_lxc for now, though I see why you have it.  Up to you.

You have your file nicely divided in sections by function.  If you were going to keep everything in this one file, I'd ask for comment lines delineating the sections--in particular, I had to think to realize that we were in the arg parsing section when I saw handle_user.  Since you are going to divide this up into files anyway, I won't worry about it now.

You are not using os.path.expanduser in handle_directories for a reason: you are using the home directory of the user that was passed in, not the current home directory.  A comment explaining this might be helpful, or might not.

The ArgumentParser might be improved if it showed in the docstring how users would use the registered subcommands.  I expect that kind of thing might fall out of the division into separate files.

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.

When I run the command's help, I get the following output:

"""
$ ./lpsetup.py -h
usage: lpsetup.py [-h] {install,update,lxc-install} ...

Create and update Launchpad development and testing environments.

optional arguments:
  -h, --help            show this help message and exit

subcommands:
  valid subcommands

{install,update,lxc-install}
                        -h, --help show subcommand help and exit
    install             Install the Launchpad environment.
    update              Update the Launchpad environment to latest version.
    lxc-install         Install the Launchpad environment inside an LXC.
"""
- I find the text "valid subcommands {install,update,lxc-install}" to be unnecessary and confusing.
- I find the placement of "-h, --help..." to be confusing too.  Perhaps put that text, written more explicitly, where the "calid subcommands" text is?  "Each subcommand accepts --h or --help to describe it."?  Also, this reminds me, and probably other people who will use lpsetup, of bzr.  They will want to do the following:

$ lpsetup help
(this could be equivalent to lpsetup -h)

$ lpsetup help install
(this could be equivalent to lpsetup install -h)
- The first (usage) line looks like you ought to be able to write "lpsetup -h install".  You can, but it shows the top-level help--not what I'd expect.  I think this usage line is confusing.  Ideally, I think, the usage would be "lpsetup {help,install,update,lxc-install} ..." or something like that.

That's it.  Nice progress!  Thank you.

review: Approve

« Back to merge proposal