[virt-tools-list] [PATCH v3 1/4] virtinst: Add vsock device type

[Marc Hartmayer]

On Tue, Dec 18, 2018 at 04:18 PM +0100, Slavomir Kaslev <kaslevs at vmware.com> wrote:
> On 12/18/18 3:59 PM, Marc Hartmayer wrote:
>
>> On Fri, Dec 14, 2018 at 03:34 PM +0100, Slavomir Kaslev <kaslevs at vmware.com> wrote:
>>> VSOCK sockets allow communication between virtual machines and the host they are
>>> running on.
>>>
>>> This patch adds vsock device support along with clitest for the new properties.
>>>
>>> Signed-off-by: Slavomir Kaslev <kaslevs at vmware.com>
>>> ---
>> […snip…]
>>
>>>
>>>   def add_guest_xml_options(geng):
>>> @@ -2577,6 +2581,23 @@ ParserPanic.add_arg(None, "model", cb=ParserPanic.set_model_cb,
>>>   ParserPanic.add_arg("iobase", "iobase")
>>>
>>>
>>> +###################
>>> +# --vsock parsing #
>>> +###################
>>> +
>> I know this blank line is everywhere in this module… but why? It
>> violates against pep8…
>>
>> Wouldn’t it make more sense to have an docstring for the class instead?
>
> It makes perfect sense to me.
>
> Since I'm not very familiar with the coding style this project uses, I
> was trying to follow the style of the surrounding code (e.g.
> https://github.com/virt-manager/virt-manager/blob/b91393e6c35b0e2903dbb50bb57a64464a7a3802/virtinst/devices/panic.py#L48
> or
> https://github.com/virt-manager/virt-manager/blob/b91393e6c35b0e2903dbb50bb57a64464a7a3802/virtinst/cli.py#L1298
> ) but I don't have any strong feelings either way.
>
> Though, if a decision is made to switch to docstrings instead, it would
> better be done in bulk (semi-automated?) over the whole project in a
> separate patch set rather than start from here.

Yep, you’re right. Would be better to start a discussion about it in a
separate thread.

[…snip]

--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294