Nir Soffer has posted comments on this change. Change subject: tox: fail make process if required tox version isn't installed. ......................................................................
Patch Set 13: (7 comments) https://gerrit.ovirt.org/#/c/59306/13/build-aux/vercmp File build-aux/vercmp: Line 31: if padding > 0: Line 32: required_version += [0] * padding Line 33: Line 34: if actual_version < required_version: Line 35: logging.debug("{} < {}".format(actual_version, required_version)) When using logging we don't format the string before logging. Use the standard way: logging.debug("%s < %s", actual_version, required_version) Line 36: sys.exit(0) Line 37: elif actual_version == required_version: Line 38: logging.debug("{} == {}".format(actual_version, required_version)) Line 39: sys.exit(1) Line 34: if actual_version < required_version: Line 35: logging.debug("{} < {}".format(actual_version, required_version)) Line 36: sys.exit(0) Line 37: elif actual_version == required_version: Line 38: logging.debug("{} == {}".format(actual_version, required_version)) Same Line 39: sys.exit(1) Line 40: else: Line 41: logging.debug("{} > {}".format(actual_version, required_version)) Line 42: sys.exit(2) Line 37: elif actual_version == required_version: Line 38: logging.debug("{} == {}".format(actual_version, required_version)) Line 39: sys.exit(1) Line 40: else: Line 41: logging.debug("{} > {}".format(actual_version, required_version)) Same Line 42: sys.exit(2) Line 43: Line 44: Line 45: def parse_args(): Line 38: logging.debug("{} == {}".format(actual_version, required_version)) Line 39: sys.exit(1) Line 40: else: Line 41: logging.debug("{} > {}".format(actual_version, required_version)) Line 42: sys.exit(2) It would be nicer to return a value instead of exiting here. This make it possible to test this function. Line 43: Line 44: Line 45: def parse_args(): Line 46: parser = argparse.ArgumentParser(description='compare actual version to ' Line 51: parser.add_argument('-v', '--verbose', help='increase output verbosity', Line 52: action='store_true', default=False) Line 53: return parser.parse_args() Line 54: Line 55: def main(): Can you move main to the start of the module? It is much nicer to read a module from top to bottom instead of from bottom to the top. Line 56: args = parse_args() Line 57: if args.verbose: Line 58: logging.basicConfig(level=logging.DEBUG) Line 59: compare_versions(args.actual_version, args.required_version) Line 54: Line 55: def main(): Line 56: args = parse_args() Line 57: if args.verbose: Line 58: logging.basicConfig(level=logging.DEBUG) Nice! It would be also useful to set a format, otherwise we will get unhelpful log format. See for example how this is done in vdsm/storage/fc-scan. Line 59: compare_versions(args.actual_version, args.required_version) Line 60: Line 61: if __name__ == '__main__': Line 55: def main(): Line 56: args = parse_args() Line 57: if args.verbose: Line 58: logging.basicConfig(level=logging.DEBUG) Line 59: compare_versions(args.actual_version, args.required_version) We should exit here with the value returned from compare_versions. Line 60: Line 61: if __name__ == '__main__': -- To view, visit https://gerrit.ovirt.org/59306 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I665025dacdd5346a5e021ac98e864f7b6461917c Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
