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.

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?