Hi Milo, Thanks for your efforts with this. I am amazed by the amount of tests you have been adding. :-) I have some comments below review needs-fixing > === added file 'HACKING' > --- HACKING 1970-01-01 00:00:00 +0000 > +++ HACKING 2013-06-20 15:51:34 +0000 > @@ -0,0 +1,17 @@ > +Tests Code Coverage > +=================== > + > +To have a nicely HTML viewable report on tests code coverage, do as follows: > + > +* Install `python-coverage` (`pip install coverage` in case you use pip) > +* Run the following command: > + > + python-coverage run -m unittest lava_tool.tests.test_suite 2>/dev/null && python-coverage html > + > +* The report will be save in a directory called `lava_tool_coverage`: open typo: save → saved > +the `index.html` file in there to see the report. > + > +Notes: > + > + * To re-run the coverage report, you have to delete the `lava_tool_coverage` > +directory first, otherwise `python-coverage` will fail. > > === modified file 'lava/config.py' > --- lava/config.py 2013-06-20 15:51:34 +0000 > +++ lava/config.py 2013-06-20 15:51:34 +0000 > @@ -16,91 +16,224 @@ > # You should have received a copy of the GNU Lesser General Public License > # along with lava-tool. If not, see . > > +""" > +Config class. > +""" > + > import atexit > -from ConfigParser import ConfigParser, NoOptionError, NoSectionError > import os > import readline > - > -__all__ = ['InteractiveConfig', 'NonInteractiveConfig'] > - > -history = os.path.join(os.path.expanduser("~"), ".lava_history") > +import sys > + > + > +from ConfigParser import ConfigParser, NoOptionError, NoSectionError > + > +__all__ = ['Config', 'InteractiveConfig'] > + > +# Store for function calls to be made at exit time. > +AT_EXIT_CALLS = set() > +# Config default section. > +DEFAULT_SECTION = "DEFAULT" > + > +HISTORY = os.path.join(os.path.expanduser("~"), ".lava_history") > try: > - readline.read_history_file(history) > + readline.read_history_file(HISTORY) > except IOError: > pass > -atexit.register(readline.write_history_file, history) > - > -config_file = (os.environ.get('LAVACONFIG') or > - os.path.join(os.path.expanduser('~'), '.lavaconfig')) > -config_backend = ConfigParser() > -config_backend.read([config_file]) > - > - > -def save_config(): > - with open(config_file, 'w') as f: > - config_backend.write(f) > -atexit.register(save_config) > - > - > -class Parameter(object): > - > - def __init__(self, id, depends=None): > - self.id = id > - self.depends = depends > - > - > -class InteractiveConfig(object): > - > - def __init__(self, force_interactive=False): > - self._force_interactive = force_interactive > +atexit.register(readline.write_history_file, HISTORY) > + > + > +def _run_at_exit(): > + """Runs all the function at exit.""" > + for call in list(AT_EXIT_CALLS): > + call() > +atexit.register(_run_at_exit) > + > + > +class Config(object): > + """A generic config object.""" > + def __init__(self): > + # The cache where to store parameters. > self._cache = {} > - > - def get(self, parameter): > - key = parameter.id > - value = None > + self._config_file = (os.environ.get('LAVACONFIG') or > + os.path.join(os.path.expanduser('~'), > + '.lavaconfig')) > + self._config_backend = ConfigParser() > + self._config_backend.read([self._config_file]) > + AT_EXIT_CALLS.add(self.save) > + > + 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: > - pass > - config_section = parameter.depends.id + '=' + self.get(parameter.depends) > + section = "{0}={1}".format(parameter.depends.id, > + self.get(parameter.depends)) > + return section > + > + def get(self, parameter, section=None): > + """Retrieves a Parameter value. > + > + The value is taken either from the Parameter itself, or from the cache, > + or from the config file. > + > + :param parameter: The parameter to search. > + :type Parameter > + :return The parameter value, or None if it is not found. > + """ > + if not section: > + section = self._calculate_config_section(parameter) > + # Try to get the parameter value first if it has one. > + if parameter.value: > + value = parameter.value > else: > - config_section = "DEFAULT" > - > - if config_section in self._cache: > - if key in self._cache[config_section]: > - return self._cache[config_section][key] > - > - prompt = '%s: ' % key > - > + value = self._get_from_cache(parameter, section) > + > + if not value: > + value = self._get_from_backend(parameter, section) > + return value > + > + def _get_from_backend(self, parameter, section): > + """Gets the parameter value from the config backed. typo backed → backend > + > + :param parameter: The Parameter to look up. > + :param section: The section in the Config. > + """ > + value = None > try: > - value = config_backend.get(config_section, key) > + value = self._config_backend.get(section, parameter.id) > except (NoOptionError, NoSectionError): > + # Ignore, we return None. > pass > - if value: > - if self._force_interactive: > - prompt = "%s[%s]: " % (key, value) > - else: > - return value > - try: > - user_input = raw_input(prompt).strip() > - except EOFError: > - user_input = None > - if user_input: > - value = user_input > - if not config_backend.has_section(config_section) and config_section != 'DEFAULT': > - config_backend.add_section(config_section) > - config_backend.set(config_section, key, value) > - > - if value: > - if config_section not in self._cache: > - self._cache[config_section] = {} > - self._cache[config_section][key] = value > - return value > + return value > + > + def _get_from_cache(self, parameter, section): > + """Looks for the specified parameter in the internal cache. > + > + :param parameter: The parameter to search. > + :type Parameter > + :return The parameter value, of None if it is not found. > + """ > + value = None > + if section in self._cache.keys(): > + if parameter.id in self._cache[section].keys(): > + value = self._cache[section][parameter.id] > + return value > + > + def _put_in_cache(self, key, value, section=DEFAULT_SECTION): > + """Insert the passed parameter in the internal cache. > + > + :param parameter: The parameter to insert. > + :type Parameter > + :param section: The name of the section in the config file. > + :type str > + """ > + if section not in self._cache.keys(): > + self._cache[section] = {} > + self._cache[section][key] = value > + > + def put(self, key, value, section=DEFAULT_SECTION): > + """Adds a parameter to the config file. > + > + :param key: The key to add. > + :param value: The value to add. > + :param section: The name of the section as in the config file. > + """ > + if (not self._config_backend.has_section(section) and > + section != DEFAULT_SECTION): > + self._config_backend.add_section(section) > + self._config_backend.set(section, key, value) > + # Store in the cache too. > + self._put_in_cache(key, value, section) > + > + def put_parameter(self, parameter, value, section=None): > + """Adds a Parameter to the config file and cache. > + > + :param Parameter: The parameter to add. > + :param value: The value of the parameter. > + """ > + if not section: > + section = self._calculate_config_section(parameter) > + self.put(parameter.id, value, section) > + > + def save(self): > + """Saves the config to file.""" > + with open(self._config_file, "w") as write_file: > + self._config_backend.write(write_file) > + > + > +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. > + > + def get(self, parameter, section=None): > + """Overrides the parent one. > + > + The only difference with the parent one, is that it will ask to type > + a parameter value in case it is not found. > + """ > + if not section: > + section = self._calculate_config_section(parameter) > + value = super(InteractiveConfig, self).get(parameter, section) > + > + if value and self._force_interactive: > + prompt = "Reinsert value for {0} [was: {1}]: ".format( I think we should save screen space and just use '{0} [{1}]: ' as prompt. People are used to that format. > + parameter.id, > + value) > else: > - raise KeyError(key) > - > -class NonInteractiveConfig(object): > - > - def __init__(self, data): > - self.data = data > - > - def get(self, parameter): > - return self.data[parameter.id] > + prompt = "Insert value for {0}: ".format(parameter.id) > + > + if not (value and parameter.asked): > + if not value or self._force_interactive: > + user_input = None > + try: > + user_input = raw_input(prompt).strip() You don't need to this right now, but reading the input should be delegated to the parameter object, so that in the (near) future we can implement custom parameter classes that have their own prompting logic (e.g. the parameter for image could first ask the image type -- prebuilt, rootfs/hwpack, then for the actual URL(s) of each component.). user_input = parameter.prompt() you can make Parameter#prompt() just return raw_input(prompt).strip() for now though, and move the logic to calculate the prompt inside the Parameter class as well. > + except EOFError: > + pass > + except KeyboardInterrupt: > + sys.exit(-1) > + > + if user_input: > + value = user_input > + self.put(parameter.id, value, section) > + return value > > === modified file 'lava/device/__init__.py' > --- lava/device/__init__.py 2013-06-20 15:51:34 +0000 > +++ lava/device/__init__.py 2013-06-20 15:51:34 +0000 > @@ -18,9 +18,14 @@ > > import re > > +from copy import deepcopy > + > +from lava.config import Config > from lava.device.templates import ( > + DEFAULT_TEMPLATE, > + HOSTNAME_PARAMETER, > KNOWN_TEMPLATES, > - DEFAULT_TEMPLATE, > + update_template, > ) > from lava.tool.errors import CommandError > > @@ -44,60 +49,38 @@ > > class Device(object): > """A generic device.""" > - def __init__(self, hostname, template): > - self.device_type = None > + def __init__(self, hostname=None, template=None): > self.hostname = hostname > - self.template = template.copy() > + self.template = deepcopy(template) > > def write(self, conf_file): > """Writes the object to file. > > :param conf_file: The full path of the file where to write.""" > with open(conf_file, 'w') as write_file: > - write_file.write(self.__str__()) > - > - 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. > + > + # 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? > + > + update_template(self.template, config) > > def __str__(self): > - self._update() > string_list = [] > for key, value in self.template.iteritems(): > - if not value: > - value = '' > string_list.append("{0} = {1}\n".format(str(key), str(value))) > return "".join(string_list) > > - def __repr__(self): > - self._update() > - return str(self.template) > - > - > -def _get_device_type_from_user(): > - """Makes the user write what kind of device this is. > - > - If something goes wrong, raises CommandError. > - """ > - try: > - dev_type = raw_input("Please specify the device type: ").strip() > - except (EOFError, KeyboardInterrupt): > - dev_type = None > - if not dev_type: > - raise CommandError("DEVICE name not specified or not correct.") > - return dev_type > - > > def get_known_device(name): > """Tries to match a device name with a known device type. > @@ -105,20 +88,9 @@ > :param name: The name of the device we want matched to a real device. > :return A Device instance. > """ > - instance = None > + instance = Device(name, DEFAULT_TEMPLATE) > for known_dev, (matcher, dev_template) in KNOWN_DEVICES.iteritems(): > if matcher.match(name): > instance = Device(name, dev_template) > - if not instance: > - dev_type = _get_device_type_from_user() > - known_dev = KNOWN_DEVICES.get(dev_type, None) > - if known_dev: > - instance = Device(name, known_dev[1]) > - else: > - print ("Device '{0}' does not match a known " > - "device.".format(dev_type)) > - instance = Device(name, DEFAULT_TEMPLATE) > - # Not stricly necessary, users can fill up the field later. > - instance.device_type = dev_type > - > + break > return instance > > === modified file 'lava/device/templates.py' > --- lava/device/templates.py 2013-06-20 15:51:34 +0000 > +++ lava/device/templates.py 2013-06-20 15:51:34 +0000 > @@ -21,22 +21,61 @@ > will be used to serialize a Device object. > """ > > +from lava.config import Config > +from lava.parameter import Parameter > + > +# The hostname parameter is always in the DEFAULT config section. > +HOSTNAME_PARAMETER = Parameter("hostname") > +DEVICE_TYPE_PARAMETER = Parameter("device_type", depends=HOSTNAME_PARAMETER) > +CONNECTION_COMMAND_PARMETER = Parameter("connection_command", > + depends=DEVICE_TYPE_PARAMETER) typo: CONNECTION_COMMAND_PARMETER → CONNECTION_COMMAND_PARAMETER (missing an A in PARAMETER) > + > DEFAULT_TEMPLATE = { > - 'device_type': None, > - 'hostname': None, > - 'connection_command': None, > + 'hostname': HOSTNAME_PARAMETER, > + 'device_type': DEVICE_TYPE_PARAMETER, > + 'connection_command': CONNECTION_COMMAND_PARMETER, > } > > # Dictionary with templates of known devices. > KNOWN_TEMPLATES = { > 'panda': { > - 'device_type': 'panda', > - 'hostname': None, > - 'connection_command': None, > + 'hostname': HOSTNAME_PARAMETER, > + 'device_type': Parameter("device_type", depends=HOSTNAME_PARAMETER, > + value="panda"), > + 'connection_command': CONNECTION_COMMAND_PARMETER, > }, > 'vexpress': { > - 'device_type': 'vexpress', > - 'hostname': None, > - 'connection_command': None, > + 'hostname': HOSTNAME_PARAMETER, > + 'device_type': Parameter("device_type", depends=HOSTNAME_PARAMETER, > + value="vexpress"), > + 'connection_command': CONNECTION_COMMAND_PARMETER, > }, > } > + > + > +def update_template(template, config): > + """Updates a template based on the values from the provided config. > + > + :param template: A template to be updated. > + :param config: A Config instance where values should be taken. > + """ > + > + # Really make sure. > + assert isinstance(config, Config) again testing the class of an object > + > + 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. > === added file 'lava/parameter.py' > --- lava/parameter.py 1970-01-01 00:00:00 +0000 > +++ lava/parameter.py 2013-06-20 15:51:34 +0000 > @@ -0,0 +1,30 @@ > +# Copyright (C) 2013 Linaro Limited > +# > +# Author: Antonio Terceiro