Nir Soffer has posted comments on this change. Change subject: contrib: schema converter ......................................................................
Patch Set 25: (4 comments) https://gerrit.ovirt.org/#/c/52864/25/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): Can you rename this class in this patch? (should not break the next patches). Line 53: Line 54: def __init__(self, paths): Line 55: self._schema = {} Line 56: try: Line 54: def __init__(self, paths): Line 55: self._schema = {} Line 56: try: Line 57: for path in paths: Line 58: with open(path) as f: Why do we need to support multiple schema files? Line 59: data = yaml.load(f) Line 60: self._schema.update({key: value for (key, value) Line 61: in data.items() if key != 'types'}) Line 62: Line 55: self._schema = {} Line 56: try: Line 57: for path in paths: Line 58: with open(path) as f: Line 59: data = yaml.load(f) data is good name for blob of bytes. But this is a dict, or actually a schema dict. Line 60: self._schema.update({key: value for (key, value) Line 61: in data.items() if key != 'types'}) Line 62: Line 63: self._schema['types'] = dict( Line 61: in data.items() if key != 'types'}) Line 62: Line 63: self._schema['types'] = dict( Line 64: self._schema.get('types', {}).items() + Line 65: data.get('types').items()) This code is not clear and very inefficient. - copy huge dict into a huge tuple (items()) - create a temporary dict from the tuple, filtering out 'types' - update an schema dict with the temporary dict - copy schema['types'] to tuple - copy new schema['types'] to tuple - create new dict from both tuples How about: self._methods = {} self._types = {} for path in paths: with open(path): schema = yaml.load(f) types = schema.pop('types') self._types.update(types) self._methods.update(schema) We never expose the underlying _schema, so there is no reason to keep the the schema in single dict. Line 66: except IOError: Line 67: raise SchemaNotFound("Unable to find API schema file") Line 68: Line 69: def get_params(self, class_name, method_name): -- 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: 25 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
