Code review comment for lp://staging/~tealeg/landscape-client/use-configobj

Revision history for this message
Geoff Teale (tealeg) wrote :

> Looks good! +1
>
> [1]
>
> +from configobj import ConfigObj
>
> We need to add a Depends entry for this in debian/control (and check that it's
> available and on all supported releases).

Done.

>
> [2]
>
> + def get_config_object(self, config_source=None):
>
> Please add a docstring and make this private, since it's not meant to be used
> by calling code.

Done.

>
> [3]
>
> + if config_source:
> + config_obj = ConfigObj(config_source)
> + else:
> + config_obj = ConfigObj(self.get_config_filename())
>
> Please prefer removing code duplication when instantiating objects, e.g.:
>
> filename = config_source or self.get_config_filename()
> config_obj = ConfigObj(filename)
>
> or:
>
> if config_source is not None:
> filename = config_source
> else:
> filename = self.get_config_filename()
> config_obj = ConfigObj(filename)

Done

« Back to merge proposal