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