Francesco Romani has posted comments on this change. Change subject: bridge: data verification ......................................................................
Patch Set 20: Code-Review-1 (9 comments) partial review, nothing big but quite a few questions, -1 for visibility only https://gerrit.ovirt.org/#/c/53919/20/lib/api/schemaapi.py File lib/api/schemaapi.py: PS20, Line 34: isinstance this will break on python3 (see a lot of docs on the internet, e.g. http://lucumr.pocoo.org/2014/1/5/unicode-in-2-and-3/) Maybe six gives us something here? what happens if we just check if it is unicode? PS20, Line 80: self._mode Maybe rename _strict_mode ? or just _strict? I prefer explicit arguments wherever possible (here seems easy to add it), but this is mostly personal taste and we overuse config anyway, so I'm not going to push for this. PS20, Line 143: keys() redundant: >>> d = {'a': 21, 'b': 42} >>> for k in d: print(k) ... a b PS20, Line 150: get_params(class_name, method_name) maybe do this once at the beginning of the method? PS20, Line 163: type(e).__name__ please use isinstance() - or add a comment explaining why you cannot PS20, Line 179: .keys() probably redundant, see above PS20, Line 231: .keys() same PS20, Line 240: .keys() same PS20, Line 292: type(e).__name__ same, please use isinstance() or explain why you cannot -- To view, visit https://gerrit.ovirt.org/53919 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id24a5e078fa92e4129d37a47593c7a167e78712e Gerrit-PatchSet: 20 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: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[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
