On 12/19/2018 04:31 AM, Marc Hartmayer wrote:
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.
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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
