Change in vdsm[master]: contrib: schema converter
nsoffer at redhat.com
nsoffer at redhat.com
Tue Feb 23 21:09:00 UTC 2016
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.kliczewski at gmail.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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: 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