On Fri, Jul 27, 2012 at 11:51 AM, Chris Evich <[email protected]> wrote: > On 07/26/2012 04:32 PM, Lucas Meneghel Rodrigues wrote: >> >> On Thu, Jul 26, 2012 at 5:20 PM, Lucas Meneghel Rodrigues >>> >>> + def managedsave(self, options="", ignore_status=False, >>> print_info=False): > > ... > >> >> ^ Ok, here is my entire objection against this patch. There's no need >> to add a public interface to the vm class, as managedsave is more of a >> management general function. So instead just call the virsh function >> in the test rather than vm.managedsave. So please send another version >> of this patchset with this fixed. >> >> Thanks for your work, >> >> Lucas > > > Just as FYI, a good rule-of-thumb here is: If the functionality will be > used by 3 or more test modules, then it's okay to put as a vm method. > Otherwise, just leave it as special-case code inside a test. Otherwise we > end up with many many many vm methods to maintain :)
It's also important to ask yourself whether the method seems to belong to a *vm* object. Now, I'm not saying we've been perfect on this regard, but it's good to think more carefully about how we're modeling those objects. After all, it's an important part of object oriented programming. -- Lucas _______________________________________________ Autotest-kernel mailing list [email protected] https://www.redhat.com/mailman/listinfo/autotest-kernel
