On Fri, Jul 27, 2012 at 1:13 PM, Francesco Banconi <[email protected]> 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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
