Adam Litke has posted comments on this change. Change subject: Add an object model to clientIF ......................................................................
Patch Set 1: (7 inline comments) Ewoud, thank you for your review! I have corrected the whitespace issues and have countered with my opinions on dict.copy() in this particular case. .................................................... File vdsm/API.py Line 27: class VM(object): Done Line 43: return self._cif.create(createParameters) Hmm, I understand why you would suggest making a copy of the dict here but there are reasons why I would suggest keeping it the way it is: 1) createParams must contain a key 'vmId' whose value matches the UUID of the currently instantiated vm object. Therefore, this modification of the dictionary serves to enforce a rule, not override user intention. 2) This dict contains other structures and is quite large. Therefore an inefficient deepcopy() would be required. Besides, the actual create method already makes alterations to the params dict. Line 72: return self._cif.migrate(params) I would propose the same reasons here for keeping it the way it is. Line 78: params['vmId'] = self._UUID ... and here. Line 373: Done Line 406: # This internal API deviates horribly in that it takes a Done Line 408: _vmString = string.join(vmList, ',') Done -- To view, visit http://gerrit.ovirt.org/984 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ee54277c87a02ac8dafe29a8761e2acf0f7397f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
