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

Revision history for this message
Milo Casagrande (milo) wrote :

Hi Antonio!

Thanks for the review.

On Tue, Jun 11, 2013 at 2:31 AM, Antonio Terceiro
<email address hidden> wrote:
> Review: Needs Fixing
>
> Hi Milo,
>
> Very nice progress here! I have some comments below to help improving it.
>
> review needs-fixing
>
> Also I wonder whether we should merge my branch into trunk already so
> you don't keep developing off the official places ... it's not like
> there will be a release before this work is done with.

As you prefer, I based my branch on yours so the diff would make sense here.
As soon as we merge that we can switch to trunk.

> if you dropped pyflakes/pep8 tests there is no point in installing them
> here.

Forgot to clean up those dependencies. Cleared them all out, and
cleaned also the bzrignore file.

> I don't think object has anything useful in its constructor.

Default code snippet/autocompletion when I create classes. I usually
do not even pay attention to that.
Cleared out.

> In _update you are changing this object, you might want to make a copy instead.

Makes sense, indeed.

>> +
>> + def write(self, where):
>> + """Writes the object to file.
>> +
>> + :param where: The full path of the file where to write."""
>> + with open(where, 'w') as f:
>> + f.write(self.__str__())
>
> renaming `where` to `file` will make the code a lot more obvious.

I used `conf_file`, I do not like using the same name as the built-in
"file" for a variable even if it is as simple as in this case (plus
there is the description just above/below).

>> +# Dictionary with key the name of a know device, and value a tuple composed of
>> +# a matcher used to guess the device type, and its associated Device class.
>> +known_devices = {
>> + 'panda': (re.compile('^.*panda.*', re.I), PandaDevice),
>> +}
>
> you might create this dynamically from a list of device type names. I don't
> see us needing one class per device type, so perhaps the last element in the
> value tuple should be a template, which could be looked up by device type name
> in lava.device.templates, or a default if the device type name does not bring
> anything.

I cleaned up this part, removing the specialized PandaDevice class and
creating "custom" templates.
Take a look of what I did was how you were thinking about.

The regex matcher is also create "on-the-fly".

> I think you cannot always assume an instance here. If you have lava-tool
> installed with Debian packages, then you don't have a local instance. The
> problem is that the dispatcher does not expose a list of configuration
> directories that we can reference. I will work on this tomorrow, and will let
> you know.

Yeah, I was looking into getting as much info as possible out of LAVA,
but for what I needed I didn't have much luck.
Indeed this part here needs a better logic, but I wanted to have the
code out there for a first review.

> I think we could keep a dictionary of templates, indexed by device type (see
> also my comments above wrt lava.device.__init__.py)

This should have been addressed. Please take a look and let me know.

> CalledProcessError will never be raised here. It's only raised by check_call
> and check_output.
>
> try this:
>
> def has_command(cmd):
> try:
> subprocess.check_call(['which', cmd], stdout=open('/dev/null', 'w'))
> return True
> except subprocess.CalledProcessError:
> return False
>
> there should be a function in the stdlib for this ...

Have no idea why I used CalledProcessError... also, have no idea for
the stdlib function to check for an available command. I remember
seeing something for Windows (not even sure if for the same purpose
actually), but am not sure for Linux...

Fixed, but I kept the single exit point (I prefer having just one
single return statement, or at least I try to do that...).
Thanks again!

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

« Back to merge proposal