Re: [Qemu-devel] [PATCH v3 3/5] Acceptance tests: add quick VNC tests

[Cleber Rosa]

On 05/30/2018 05:31 PM, Eduardo Habkost wrote:
> On Wed, May 30, 2018 at 05:03:56PM -0400, Cleber Rosa wrote:
>> On 05/30/2018 04:00 PM, Eduardo Habkost wrote:
> [...]
>>       [...]   Now, in addition to this very personal, and thus
>> insignificant botheration, it restricts API design.  By signaling to
>> users that this is valid:
>>
>>   self.vm.add_args(...).launch(...).shutdown()
>>
>> We'd be restricted on the design of, say, migrate().  Even if it sounds
>> sensible to return a boolean indicating migration success, for
>> consistency, it'd require a "return self".
> [...]
>>> I don't see why this would be an argument against making this
>>> possible:
>>>
>>>   self.vm.add_args('-nodefaults', '-S').launch()
>>
>> Right, it's *mostly* style.  But, it suggests that all methods work
>> similarly and thus restricts API design as mentioned before.
> [...]
>>> They are more readable than the complex method chaining examples
>>> you have shown, but I don't see any argument against:
>>>
>>>   self.vm.add_args(...).launch()
>>
>> Right.  This one looks nice indeed.  *My* issues are related to
>> expectations (all methods support this?), consistency (all methods
>> should support this!) and the design limitations it brings.
>>
> 
> These are good points.
> 
> I believe it would be a reasonable convention to make methods
> that change QEMU configuration return self (because they are
> often called one after another), but other methods like launch()
> migrate(), and shutdown() don't have to.
> 
> So this would work:
> 
>   self.vm.add_args(...).add_drive(...).launch()
> 
> But this doesn't have to:
> 
>   self.vm.add_args(...).launch().migrate().shutdown()
> 

Looks like a sane general rule.  Let's revisit this when new add_*()
methods are added.

- Cleber.