Code review comment for lp:~adam-collard/landscape-client/config-file-sysinfo

> [1]
>
> This code:
>
> 51 + if os.path.isfile(self.config) and os.access(self.config,
> os.R_OK):
> 52 + self.load_configuration_file(self.config)
> 53 + elif not accept_nonexistent_config:
> 54 + sys.exit("error: config file %s can't be read" %
> self.config)
>
>
> looks a lot like this code:
>
> 77 + if os.path.isfile(config_filename):
> 78 + if not os.access(config_filename, os.R_OK):
> 79 + errors.append("error: config file %s can't be
> read" %
> 80 + config_filename)
> 81 + else:
> 82 + self.load_configuration_file(config_filename)
> 83 + break
>
> I had trouble seeing the difference at first. Only after re-reading the code
> several
> time I spotted what was different.

Yeah sorry about that. I'll add some comments to ease readability too.

>
> I'd suggest being a bit more explicit and write the whole think like this:
>
> if (self.config and not os.path.isfile(self.config)
> and not accept_nonexistent_config):
> sys.exit("error: config file %s doesn't exist" % self.config)
>
> if self.config:
> config_filenames = [self.config]
> else:
> config_filenames = self.default_config_filenames
>
> errors = []
> for config_filename in config_filenames:
> if os.path.isfile(config_filename):
> if not os.access(config_filename, os.R_OK):
> errors.append("error: config file %s can't be read" %
> config_filename)
> else:
> self.load_configuration_file(config_filename)
> break
> else:
> if errors and not accept_nonexistent_config:
> sys.exit("\n".join(errors))
>
>
> I.e., first check whether self.config exists, and then do the general checking
> that was already in place. What do you think?

I'm -0.5 on the suggestion - I don't like checking for it being a readable file twice. Once we've got past the first if we've already checked that self.config is a readable file.

I think how I have it now is more explicit, even if more verbose. There are two different behaviours for a specified configuration file vs. default configuration files, and those behaviours are in two separate blocks.

An alternative would be to try...except here and have different ways of handling the exception