Code review comment for lp:~milo/lava-tool/lava-168

Hi Antonio,

thanks for going through the review!

On Tue, Jun 18, 2013 at 9:13 PM, Antonio Terceiro
<email address hidden> wrote:
> Review: Needs Fixing
>
> Hi Milo,
>
> Thanks for this. Some of my comments are related to the questions we already
> talked about earlier today, but I will also mention them here so others can
> also follow.
>
> review needs-fixing
>
>> === added file 'lava/base_command.py'
>> --- lava/base_command.py 1970-01-01 00:00:00 +0000
>> +++ lava/base_command.py 2013-06-17 13:33:27 +0000
>
> I would put this somewhere like lava/helper/command.py instead

Agree.
This is already done with the latest push.

I also refactored all the calls that made use of the lava-dispatcher
into lava/helper/dispatcher.py, and updated all the tests too.

> Maybe when we start working on the other items (testdef/script) we realize that
> we can move all of them in the lava.helper namespace instead of creating
> lava.testdef and lava.script.
>
>> @@ -0,0 +1,80 @@
>> +# Copyright (C) 2013 Linaro Limited
>> +#
>> +# Author: Milo Casagrande <email address hidden>
>> +#
>> +# This file is part of lava-tool.
>> +#
>> +# lava-tool is free software: you can redistribute it and/or modify
>> +# it under the terms of the GNU Lesser General Public License version 3
>> +# as published by the Free Software Foundation
>> +#
>> +# lava-tool is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU Lesser General Public License
>> +# along with lava-tool. If not, see <http://www.gnu.org/licenses/>.
>> +
>> +"""Base command class common to lava commands series."""
>> +
>> +import subprocess
>> +
>> +from lava.config import InteractiveConfig
>> +from lava.tool.command import Command
>> +from lava.tool.errors import CommandError
>> +
>> +
>> +class BaseCommand(Command):
>> + """Base command class for all lava commands."""
>> + def __init__(self, parser, args):
>> + super(BaseCommand, self).__init__(parser, args)
>> + self.config = InteractiveConfig(
>> + force_interactive=self.args.interactive)
>> +
>> + @classmethod
>> + def register_arguments(cls, parser):
>> + super(BaseCommand, cls).register_arguments(parser)
>> + parser.add_argument("-i", "--interactive",
>> + action='store_true',
>> + help=("Forces asking for input parameters even if "
>> + "we already have them cached."))
>
> As we talked ealier, I think it's better to do it the other way around: assume
> interactivity always, and have a --non-interactive option to force not asking
> for input.

This one is done too.

> I think we should move all the user interaction asking for input to the
> config/parameter classes. This way we use the existing mechanism to store
> answers in configuration and ease users' life.

Agree here too, but I guess there will be some refactoring involved,
and the code review here might become quite big.
Not sure if it is better to merge this first and then work on
refactoring it later (as I write I already started working on this,
but the Config class needs to be modified).

>> === modified file 'lava/job/tests/test_commands.py'
>> def setUp(self):
>> - self.tmpdir = mkdtemp()
>> + # Fake the stdout.
>> + self.original_stdout = sys.stdout
>> + sys.stdout = open("/dev/null", "w")
>> + self.original_stderr = sys.stderr
>> + sys.stderr = open("/dev/null", "w")
>> + self.original_stdin = sys.stdin
>
> Are you not touching stdin on purpose?

Yes, I let single tests modify it if necessary, but keep it here to
restore at the end.

> maybe you have to do some rebasing before we merge this branch here, but the
> above snippet looks to me like you are deleting one test class that is already
> there (JobSubmitTest).

I think I removed too much. It has been added back now (and refactored too).

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs