yellow team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

On Fri, Jul 27, 2012 at 1:13 PM, Francesco Banconi
<francesco.banconi@xxxxxxxxxxxxx> wrote:
> Thanks for all your comments Benji. Replies follow.

[snip lots of good stuff that doesn't need comment]

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

Could we have a handler that sets namespace.is_interactive to False if
there is otherwise no value?  Hmm, I can't check right now, but
doesn't argparse's add_option take a "default" argument.  That seems
like it would work.

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

I'm worried that someone will add a step that does something dramatic
-- say "delete the entire hard disk" for effect -- and forgets to add
a description.  Maybe we could have a foo.description = None so if
there is really no description we have to be explicit about it.

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

I would like at least one blank line separating them.

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

Cool.
-- 
Benji York

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.