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
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.
>> + self.__ str__() )
>> + 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(
>
> 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 '^.*panda. *', re.I), PandaDevice), templates, or a default if the device type name does not bring
>> +# a matcher used to guess the device type, and its associated Device class.
>> +known_devices = {
>> + 'panda': (re.compile(
>> +}
>
> 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.
> 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 __init_ _.py)
> also my comments above wrt lava.device.
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 check_call( ['which' , cmd], stdout= open('/ dev/null' , 'w')) CalledProcessEr ror:
> and check_output.
>
> try this:
>
> def has_command(cmd):
> try:
> subprocess.
> return True
> except subprocess.
> return False
>
> there should be a function in the stdlib for this ...
Have no idea why I used CalledProcessEr ror... 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