Code review comment for lp://staging/~codehelp/lava-dispatcher/pep8

> Thanks for putting this together!
>
> Most of the changes look fine. I have some comments below that need to
> be addressed we merge this.
> review needs-fixing
>
> > === modified file 'lava_dispatcher/actions/lava_android_test.py'
> > raise TimeoutError(
> > - "The test case(%s) on device(%s) times out" % (
> > - test_name,
> session.dev_name))
> > + "The test case(%s) on device(%s) times out" % (
> > + test_name, session.dev_name))
>
> Please use the opportunity to fix a typo here: s/times/timed/

Done.

> > - def run(self, commands=[], command_file=None, parser=None, timeout=-1):
> > + def run(self, commands=None, command_file=None, parser=None,
> timeout=-1):
> > + if not commands:
> > + commands = []
>
> This should is not actually needed, since commands is only used below inside a
> `if commands:`. Maybe it makes clear that commands is expected to be a list,
> though.

If something other than a list is passed, the function will fail with or without the
added check. I think I'll remove the if and add a docstring instead.

> > === modified file 'lava_dispatcher/actions/lava_test_shell.py'
> > - 'testdef': {'type': 'string',
> > - 'optional': True}
> > - },
> > + {'git-repo': {'type': 'string',
> > + 'optional': True},
> > + 'bzr-repo': {'type': 'string',
> > + 'optional': True},
> > + 'revision': {'type': 'string',
> > + 'optional': True},
> > + 'testdef': {'type': 'string',
> > + 'optional': True}
> > + },
>
>
> The indentation of this part does not look right at all.

Fixed. It needed a double indent in each case:

                                        {'git-repo': {'type': 'string',
                                                'optional': True},

> > === modified file 'lava_dispatcher/device/target.py'
> > ubuntu_deployment_data = {
> > 'TESTER_PS1': "linaro-test [rc=$(echo \$?)]# ",
> > 'TESTER_PS1_PATTERN': "linaro-test \[rc=(\d+)\]# ",
> > @@ -86,7 +85,7 @@
> > """
> > raise NotImplementedError('power_on')
> >
> > - def deploy_linaro(self, hwpack, rfs):
> > + def deploy_linaro(self, hwpack, rfs, bootloader):
> > raise NotImplementedError('deploy_image')
> >
> > def deploy_android(self, boot, system, userdata):
> > @@ -130,14 +129,14 @@
> > """ Powers on the target, returning a CommandRunner object and will
> > power off the target when the context is exited
> > """
> > - proc = runner = None
> > + self.proc = runner = None
> > try:
> > - proc = self.power_on()
> > - runner = self._get_runner(proc)
> > + self.proc = self.power_on()
> > + runner = self._get_runner(self.proc)
> > yield runner
> > finally:
> > - if proc and runner:
> > - self.power_off(proc)
> > + if self.proc and runner:
> > + self.power_off(self.proc)
> >
> > def _get_runner(self, proc):
> > from lava_dispatcher.client.base import CommandRunner
>
> it looks like you are changing the logic here. Did you already test this
> change?

It's confusing at this point - this change is linked with the _customize_bootloader function which uses a self.proc but that proc is never defined, using that proc in an expect.

I'll back this change out and put a FIXME in instead.

> > === modified file 'lava_dispatcher/device/vexpress.py'
> > --- lava_dispatcher/device/vexpress.py 2013-04-05 21:58:28 +0000
> > +++ lava_dispatcher/device/vexpress.py 2013-07-16 16:30:42 +0000
> > @@ -27,6 +27,7 @@
> > from lava_dispatcher.device.master import MasterImageTarget
> > from lava_dispatcher.errors import CriticalError
> >
> > +
> > class VexpressTarget(MasterImageTarget):
> >
> > def __init__(self, context, config):
> > @@ -37,7 +38,7 @@
> > if (self.config.uefi_image_filename is None or
> > self.config.vexpress_uefi_path is None or
> > self.config.vexpress_uefi_backup_path is None or
> > - self.config.vexpress_usb_mass_storage_device is None):
> > + self.config.vexpress_usb_mass_storage_device is None):
> >
> > raise CriticalError(
> > "Versatile Express devices must specify all "
>
> This does not look right to me.

Correct, the two lines above also need the indent. I'll fix that.

Thanks, I'll update the merge proposal.