Change in vdsm[master]: virt: Vm._getUnderlyingSoundDeviceInfo() moved to core.py
mzamazal at redhat.com
mzamazal at redhat.com
Thu Feb 25 15:50:10 UTC 2016
Milan Zamazal has posted comments on this change.
Change subject: virt: Vm._getUnderlyingSoundDeviceInfo() moved to core.py
......................................................................
Patch Set 2:
(2 comments)
https://gerrit.ovirt.org/#/c/53616/2/vdsm/virt/vmdevices/core.py
File vdsm/virt/vmdevices/core.py:
Line 246:
> naming: what about "update_device_info"?
> this also feels like a staticmethod.
I'm not a big fan of static methods. I can see little benefit in using them and they are less flexible. E.g. even if nothing else, I prefer calling a utility method inside another method by cls._method() rather than ThatClassName._method(). And I can imagine doing something like
for device_class in devices:
device_class.update_device_info(...)
in future. But technically we can make this method static now, so I'm only lightly opposed to the suggestion.
> naming: what about "update_device_info"?
Good idea.
Line 252: :type domain: :class:`DomainDescriptor` instance
Line 253: :param sound_devices: current sound devices for update
Line 254: :type sound_devices: dictionary
Line 255: :param device_conf: current device configuration for update
Line 256: :type device_conf: dictionary
> I believe it is sufficient if you have this doc only in the base method, to
I agree.
Line 257:
Line 258: """
Line 259: for x in domain.get_device_elements('sound'):
Line 260: alias = x.getElementsByTagName('alias')[0].getAttribute('name')
--
To view, visit https://gerrit.ovirt.org/53616
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I05cb24e0a9776e21bd2546d62a56554d74559c61
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list