Change in vdsm[master]: contrib: schema converter

piotr.kliczewski at gmail.com piotr.kliczewski at gmail.com
Tue Mar 29 09:05:32 UTC 2016


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 <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Sahina Bose <sabose at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list