On Tue, Dec 18, 2018 at 04:18 PM +0100, Slavomir Kaslev <kaslevs@xxxxxxxxxx> wrote:
> On 12/18/18 3:59 PM, Marc Hartmayer wrote:
>
>> On Fri, Dec 14, 2018 at 03:34 PM +0100, Slavomir Kaslev <kaslevs@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>> […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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
