Piotr Kliczewski has posted comments on this change. Change subject: contrib: schema converter ......................................................................
Patch Set 27: (8 comments) https://gerrit.ovirt.org/#/c/52864/27/lib/api/schemaapi.py File lib/api/schemaapi.py: Line 37: from vdsm import constants Line 38: Line 39: localpath = os.path.dirname(__file__) Line 40: installedpath = constants.P_VDSM_RPC Line 41: for directory in localpath, installedpath: > This works but very confusing - please use: Done Line 42: path = os.path.join(directory, schema_name + '.yaml') Line 43: # we use source tree and deployment directory Line 44: # so we need to check whether file exists Line 45: if os.access(path, os.R_OK): Line 41: for directory in localpath, installedpath: Line 42: path = os.path.join(directory, schema_name + '.yaml') Line 43: # we use source tree and deployment directory Line 44: # so we need to check whether file exists Line 45: if os.access(path, os.R_OK): > Do you want to test if the file exists, or accessible? If you want to check It is not used for checking existence. It is used to determine whether we are using it during build or runtime. We want to use the first one which following check passes. Line 46: return path Line 47: Line 48: raise SchemaNotFound("Unable to find API schema file in %s or %s", Line 49: localpath, installedpath) Line 42: path = os.path.join(directory, schema_name + '.yaml') Line 43: # we use source tree and deployment directory Line 44: # so we need to check whether file exists Line 45: if os.access(path, os.R_OK): Line 46: return path > This code will skip *existing* schema but unreadable in one directory, and Please see my previous comment. Line 47: Line 48: raise SchemaNotFound("Unable to find API schema file in %s or %s", Line 49: localpath, installedpath) Line 50: Line 56: self._types = {} Line 57: try: Line 58: for path in paths: Line 59: with open(path) as f: Line 60: loaded_schema = yaml.load(f) > Once you loaded the schema, you can close the file. The current code will c Done Line 61: types = loaded_schema.pop('types') Line 62: self._types.update(types) Line 63: self._methods.update(loaded_schema) Line 64: except IOError: Line 61: types = loaded_schema.pop('types') Line 62: self._types.update(types) Line 63: self._methods.update(loaded_schema) Line 64: except IOError: Line 65: raise SchemaNotFound("Unable to find API schema file") > Are you sure this always raises IOError? and never OSError? the safe way to Done Line 66: Line 67: def get_params(self, class_name, method_name): Line 68: return self.get_method(class_name, method_name).get('params', []) Line 69: Line 64: except IOError: Line 65: raise SchemaNotFound("Unable to find API schema file") Line 66: Line 67: def get_params(self, class_name, method_name): Line 68: return self.get_method(class_name, method_name).get('params', []) > This assumes that get_method return always a dict... Will handle AttributeError Line 69: Line 70: def get_ret_param(self, class_name, method_name): Line 71: return self.get_method(class_name, method_name).get('return', {}) Line 72: Line 70: def get_ret_param(self, class_name, method_name): Line 71: return self.get_method(class_name, method_name).get('return', {}) Line 72: Line 73: def get_method(self, class_name, method_name): Line 74: return self._methods.get('%s.%s' % (class_name, method_name)) > This may return None... As in other comments. Line 75: Line 76: def get_type(self, type_name): Line 73: def get_method(self, class_name, method_name): Line 74: return self._methods.get('%s.%s' % (class_name, method_name)) Line 75: Line 76: def get_type(self, type_name): Line 77: return self._types.get(type_name) > Why do you use _types.get(name) and not _types[name]? Will fix. -- To view, visit https://gerrit.ovirt.org/52864 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3921cebb7f550f63849f3bc5c80636b6e9495c92 Gerrit-PatchSet: 27 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Darshan N <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Milan Zamazal <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
