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

Hello Antonio!

Thanks for going through this!

On Fri, Jun 21, 2013 at 1:40 PM, Antonio Terceiro
<email address hidden> wrote:
>
>> +class InteractiveConfig(Config):
>> + """An interactive config.
>> +
>> + If a value is not found in the config file, it will ask it and then stores
>> + it.
>> + """
>> + def __init__(self, force_interactive=True):
>> + super(InteractiveConfig, self).__init__()
>> + self._force_interactive = force_interactive
>> +
>> + def _calculate_config_section(self, parameter):
>> + """Calculates the config section of the specified parameter.
>> +
>> + :param parameter: The parameter to calculate the section of.
>> + :type Parameter
>> + :return The config section.
>> + """
>> + section = DEFAULT_SECTION
>> + if parameter.depends:
>> + # This is mostly relevant to the InteractiveConfig class.
>> + # If a parameter has a dependency we do as follows:
>> + # - Get the dependency cached value
>> + # - Get the dependency value from the config file
>> + # - If both are None, it means the dependency has not been inserted
>> + # yet, and we ask for it.
>
> I have the impression that just calling get() would already to this.
>
>> + depend_section = self._calculate_config_section(parameter.depends)
>> +
>> + cached_value = self._get_from_cache(parameter.depends,
>> + depend_section)
>> + config_value = self._get_from_backend(parameter.depends,
>> + depend_section)
>> +
>> + # Honor the cached value.
>> + value = cached_value or config_value
>> + if not value:
>> + value = self.get(parameter.depends)
>> + parameter.depends.asked = True
>> + section = "{0}={1}".format(parameter.depends.id, value)
>> + return section
>
> Also, I don't understand why you need to override this method. I can't see why
> the logic for determining the section to write a config entry to would be
> different on interactive config wrt to plain config - as long as get() does the
> right thing.

Yeah, this is something I forgot to remove.
I had a problem in the device template and how I was "copying" the
default template, and when trying to figure out what was happening, I
override that method.
The two gets (from cache and config) were done to avoid multiple calls
to the _calculate_config_section method (that is why I added the
section parameter to get()).

>> -
>> - def _update(self):
>> - """Updates the template with the values specified for this class.
>> -
>> - Subclasses need to override this when they add more specific
>> - attributes.
>> + write_file.write(str(self))
>> +
>> + def update(self, config):
>> + """Updates the Device object values based on the provided config.
>> +
>> + :param config: A Config instance.
>> """
>> - # This is needed for the 'default' behavior. If we matched a known
>> - # device, we do not need to update its device_type, since its already
>> - # defined in the template.
>> - if self.device_type:
>> - self.template.update(hostname=self.hostname,
>> - device_type=self.device_type)
>> - else:
>> - self.template.update(hostname=self.hostname)
>> + if not isinstance(config, Config):
>> + raise CommandError("Error updating the device values.")
>
> I don't think that we should be testing class here. As long as the config
> object responds to the API we need, we shouldn't care which class it is.

Sounds good. It was more a "defensive programming" approach than anything else.

>> +
>> + # We should always have a hostname, since it defaults to the name
>> + # given on the command line for the config file.
>> + if self.hostname:
>> + config._put_in_cache(HOSTNAME_PARAMETER.id, self.hostname, "DEFAULT")
>
> Calling private methods of other objects? :-)
>
> Why don't you just use put()? Does this class has the authority to mandate
> how/where the config should store this value?

Hmmm... I guess you picked the emailed version of the diff. This was
already fixed.
I used the private method when working on that part and debugging it,
but it was fixed with another commit.

>> + def update(data):
>> + """Internal recursive function."""
>> + if isinstance(data, dict):
>> + keys = data.keys()
>> + elif isinstance(data, list):
>> + keys = range(len(data))
>> + else:
>> + return
>> + for key in keys:
>> + entry = data[key]
>> + if isinstance(entry, Parameter):
>> + data[key] = config.get(entry)
>> + else:
>> + update(entry)
>> +
>> + update(template)
>
> a template is ... well, a template. :-) Instead of modifying template objects,
> we should be creating *new* objects based on them. I would rename
> update_template to expand_template, then call the inner update() function on a
> copy of the template argument.

Function renamed.
For the copy part, this is already done in the classes that make use
of the template.
In this case it is the Device constructor that creates a copy of the
template (a deep copy).

Or do we really want to delegate this to the expand_template and
return the updated dictionary?
Also, that function at the moment is duplicated: one definition is in
lava/job/__init__.py, the other in lava/device/templates.py, but I'm
not completely sure where we can put this (maybe somewhere in the
lava/helper/ module?).

> Classes that only carry data with no associated behavior should always raise an
> eyebrow (see my comments above)
>
> I know this one is on me. :-)

:-)
Will move the user input logic here. :-)

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