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

[Cole Robinson]

On 12/19/2018 04:31 AM, Marc Hartmayer wrote:
> 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.
> 

I use comment blocks like that to visually break up major areas of the 
code. For me at least it helps with quick visual navigation. In the 
cli.py area it doesn't matter as much, they could be doc strings of the 
parser classes; their main purpose is to grep for the argument name 
(--vsock) which is just as well served with a docstring. docstring fit 
nicer in cli.py after Marc's patches too

Thanks,
Cole