Francesco Romani has posted comments on this change. Change subject: hostdev: use specific device classes in HostDevice ......................................................................
Patch Set 7: (1 comment) +0: I like the improvements and I like the general direction, but I don't want to commit _yet_ to this solution. https://gerrit.ovirt.org/#/c/57963/7/vdsm/virt/vmdevices/hostdevice.py File vdsm/virt/vmdevices/hostdevice.py: PS7, Line 314: def __new__(cls, conf, log, **kwargs): : device_params = get_device_params(kwargs['device']) : device = cls._DEVICE_MAPPING[ : device_params['capability']](conf, log, **kwargs) : return device This is a massive improvement over the delegation approach. I'm a bit concerned about the usage of __new__. I'd like to avoid _too_ dynamic/clever code. Anyway this specific use seems still acceptable. I'm a fan of this idiom @classmethod def from_device_params(cls, conf, log, **kwargs): device_params = get_device_params(kwargs['device']) device = cls._DEVICE_MAPPING[ device_params['capability']](conf, log, **kwargs) return device but I concur that in this case we have little benefit, beside personal taste. TL;DR: I don't have any concrete reason against __new__, I'm just asking (not requiring) to evaluate possible alternatives. From my POV the class hierarchy approach could work, I've nothing against it either. -- To view, visit https://gerrit.ovirt.org/57963 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2734b7ec789c0ce57b64d7699cbe967c774bb608 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Milan Zamazal <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
