yellow team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

I like the dry run and description functionality.

The use of $templates for the descriptions was a good choice.  It keeps
them simpler than having to have a function for each, but provides the
dynamism we need to be able to include runtime values.

We might want an assertion that no un-replaced substitutions are left in
the description after substitution takes place.  I wouldn't be surprised
if we typo a name now and then and it would keep those from persisting
very long.

The careful indention of description strings only for them to be
dedented in the future seems odd.  This:

fetch.description = """Set up a Launchpad repository in $repository,
    retrieving the source code from $source."""

or this:

fetch.description = ('Set up a Launchpad repository in $repository,'
    'retrieving the source code from $source.')

better matches prevailing Python style than this:

fetch.description = """
    Set up a Launchpad repository in $repository,
    retrieving the source code from $source.
"""

I don't feel strongly about it, but normally see dry-run options spelled
"--dry-run" I don't recall seeing one spelled "--dry".

Having the namespace attribute as "dry" is a bit terse.  Spelling it
"dry_run" would help when reading the code.

I'm surprised that the bit about inverting the boolean if store_false is
used (in lpsetup/argparser.py, line 19 of the diff) is necessary.  I
didn't look deeply enough into it to understand why, but I just wanted
to point it out in case it made you realize anything.

Having to do self.is_interactive(namespace) instead of something like
namespace.is_interactive is unfortunate.  I think I see why: because not
all commands have the option, so sometimes the attribute won't be there.
I wonder if it would be better to always have the attribute available,
even if it can not be true would be better.

The fact that steps can have empty descriptions feels like a trap.
Perhaps it should be an error for that to happen.

The confirm function is never called with a question other than "Do you
want to proceed?" and the "suffix" parameter is never used so it could
be simplified.  Some tests would go away too.

It feels like there should be some whitespace between the step function
definitions and the description attribute assignment for the functions.
I don't know if the pep8 formatting checker will warn about them or not.

The way descriptions are presented is a little hard to read.  Here is
some output from my system:

Update your system and install necessary deb packages (ssh bzr
apache2.2-common).
Create the user benji if it does not exist.
Create Apache document roots for launchpad and enable required Apache
modules (proxy proxy_http rewrite ssl deflate headers).
Set up hosts file for Launchpad (/etc/hosts).
Set up the user's ssh directory (/home/benji/.ssh).
Create, if it does not exist, the ssh key /home/benji/.ssh/id_rsa.
Authorize this key for the user benji.
Add bazaar.launchpad.net to known hosts.
Set up bazaar authentication: Benji York <benji.york@xxxxxxxxxxxxx>.
Set up Launchpad user id: benji.
Add required APT repositories and install Launchpad dependencies:
launchpad-database-dependencies-9.1 launchpad-developer-dependencies apache2
apache2-mpm-worker libapache2-mod-wsgi.

How about something like this instead?

* Update your system and install necessary deb packages (ssh bzr
  apache2.2-common).
* Create the user benji if it does not exist.
* Create Apache document roots for launchpad and enable required Apache
  modules (proxy proxy_http rewrite ssl deflate headers).
* Set up hosts file for Launchpad (/etc/hosts).
* Set up the user's ssh directory (/home/benji/.ssh).
* Create, if it does not exist, the ssh key /home/benji/.ssh/id_rsa.
* Authorize this key for the user benji.
* Add bazaar.launchpad.net to known hosts.
* Set up bazaar authentication: Benji York <benji.york@xxxxxxxxxxxxx>.
* Set up Launchpad user id: benji.
* Add required APT repositories and install Launchpad dependencies:
  launchpad-database-dependencies-9.1 launchpad-developer-dependencies
  apache2 apache2-mpm-worker libapache2-mod-wsgi.

If we wanted to be extra fancy in get_step_description we could check
the current terminal width and wrap to that.

-- 
https://code.launchpad.net/~frankban/lpsetup/interactive-execution/+merge/117062
Your team Yellow Squad is requested to review the proposed merge of lp:~frankban/lpsetup/interactive-execution into lp:lpsetup.