Nir Soffer has posted comments on this change. Change subject: contrib: schema converter ......................................................................
Patch Set 14: (5 comments) https://gerrit.ovirt.org/#/c/52864/14/lib/api/Makefile.am File lib/api/Makefile.am: Line 31: $(NULL) Line 32: Line 33: dist_vdsmpyrpc_PYTHON = \ Line 34: __init__.py \ Line 35: schemaapi.py\ schemaapi is strange. I would merge this module into vdsmapi.py. Line 36: vdsmapi.py \ Line 37: $(NULL) Line 38: Line 39: dist_vdsmrpc_DATA = \ https://gerrit.ovirt.org/#/c/52864/14/lib/api/schemaapi.py File lib/api/schemaapi.py: Line 48: raise SchemaNotFound("Unable to find API schema file in %s or %s", Line 49: localpath, installedpath) Line 50: Line 51: Line 52: class SchemaCache(object): Why do we need the cache? Or this is actually a Schema class, because we want to control how self._schema is accessed? It would be nicer lf we create the instance with path to the schema, and let the caller find the schema. path = schemaapi.find_schema() schema = schemaapi.Schema(path) Now we can test this class by creating with fake (and small) schema. Line 53: Line 54: def __init__(self): Line 55: try: Line 56: with open(find_schema()) as f: https://gerrit.ovirt.org/#/c/52864/14/lib/api/vdsmapi.py File lib/api/vdsmapi.py: Line 87: return tokens[0], tokens[1:] Line 88: Line 89: Line 90: def evaluate(string): Line 91: return parse(list(map(lambda x: x, tokenize(string))))[0] Why this change is needed? map return a list, you don't need to create another list, but this should actually be replaced with list comprehension. Line 92: Line 93: Line 94: def parse_schema(fp): Line 95: exprs = [] Line 251: if _api_info is None or schema not in _api_info: Line 252: _load_api_info(schema) Line 253: return _api_info[schema] Line 254: Line 255: Why the next function moved from the old location? Line 256: def strip_stars(items): Line 257: """ Line 258: A symbol prepended with '*' means the symbol is optional. Strip this Line 259: when looking up the symbol documentation. https://gerrit.ovirt.org/#/c/52864/14/tests/schemaValidationTest.py File tests/schemaValidationTest.py: Line 54: def test_verify_schema(self): Line 55: apiobj = self._get_api('API') Line 56: self._validate(apiobj) Line 57: Line 58: @SkipTest Better mark as @brokentest("explain why we must skip this now...") Line 59: def test_verify_gluster_schema(self): Line 60: if _glusterEnabled: Line 61: apiobj = self._get_api('gapi') Line 62: self._validate(apiobj) -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> 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/mailman/listinfo/vdsm-patches