yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Thanks for all your comments Benji. Replies follow.

> 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.

Exactly.

> 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.

Didn't thought about typos. I think what you are suggesting can be implemented just replacing Template.safe_substitute with Template.substitute, that raises a KeyError or a ValueError if placeholders are missing. I used safe_substitute to ensure we always have a string even if the context is missing, but your typo argument convinced me otherwise.

> 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 will change the style of the descriptions.

> 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.

Agreed.

> 
> 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.

Unfortunately that's necessary. Normally you have flags like:

parser.add_argument('--dry', action='store_true')

And the old code to regenerate the args from the namespace worked in this case, because the logic was:
if the value is boolean, and the value is True, then add the corresponding argument

But what about:

parser.add_argument('--yes', action='store_false', dest='interactive')

In this case, following the logic above, --yes is not re-added to the arguments, because interactive will be False.

That's why, when store_false is used, we need to invert the logic: if value if False, add the argument. To keep the existing code as is, I decided to just invert the value before the check.

> 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.

namespace.interactive is False when --yes is provided. command.is_interactive() also checks for subcommand.has_interactive_run. I agree that, if has_interactive_run is False, then namespace.interactive is not defined, so we could simplify that like:

is_interactive = getattr(namespace, 'interactive', False)

This can be done in the run() method, or, maybe better, in init_namespace. 

I decided to create a method to allow sub commands to override that method and decide themselves if they are interactive. However, this machinery is not used, and, as I think you suggested, having the calculated value in the namespace could be nice. Having this context, what you'd suggest?

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

I am confused here, could you please deepen this argument? Do you want steps' descriptions to be mandatory? Consider inithost.initialize_lxc: this step has no description because it only modifies the container. So, when collecting descriptions, this step is just ignored.

> 
> 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.

Ok.

> 
> 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.

I don't know either, but, AFAICT, tarmac doesn't complain about the steps 
already defining their name like that in tests.examples.

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

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

These suggestions definitely improve the description readability, I will try 
to add these changes in this branch.

-- 
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.