Nir Soffer 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: 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): 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 if the file exists, use os.path.exists(). If the file exists and not readable to vdsm we should blow up loudly, not skip the file silently. 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 return the next. We should return the first and fail when trying to read it. 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 close the file only when finishing the update. This is very interesting in this context, but a general bad practice. Should be: with open(...) as f: schema = yaml.load(f) update ... 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 catch both is to use EnvironmentError, their parent class. 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... It will raise AttributeError when you try None.get('params', []) 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... 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]? The first form will return None, leading to unclear AttributeError when someone try to call methods on None, which the seconds form wil fail in a very clear way with KeyError. Typically it is much easier to debug, and it is also faster. Same for other accessors. -- 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
