Change in vdsm[master]: replace configure_libvirt.py with python code.

Alon Bar-Lev alonbl at redhat.com
Thu May 15 21:25:03 UTC 2014


Alon Bar-Lev has posted comments on this change.

Change subject: replace configure_libvirt.py with python code.
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.ovirt.org/#/c/27298/9/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 446:     def _getPersistedFiles(self):
Line 447:         return [
Line 448:             cfile['path'] for cfile in self.FILES.values()
Line 449:             if cfile['persisted']
Line 450:         ]
> The standard way to indent this kind of code:
I must!!!!!!

standard format is not standard... and drawing code is bad practise.

both probably pass this famous pep8 tool, while the notation we use at engine and what used here is much easier to manage as it is consistent.

1. each statement in own block.

2. each block is single fixed indent.

your notation is based on statement length, so:

 function_name(parameter1, parameter2,
                       parameter3, parameter3)

now, you rename function_name to func_name, and get:

 func_name(parameter1, parameter2,
                       parameter3, parameter4)

so you need to patch the 2nd line as well to be align... which is *BAD* practise...

now, you want to add paramer5, so you:

 func_name(parameter1, parameter2,
                  parameter3, parameter4,
                  parameter5)

again, changing 2 lines again without any reason.

the notation of:

 function_name(
     parameter1,
     parameter2,
     parameter3,
     parameter4,
 )

solves all the above issues and more, for example can comment specific parameter:

 function_name(
     parameter1,
     parameter2,  # hello
     parameter3,
     parameter4,
 )

it is consistent, simple and flexible notation, no manual drawing code, suitable to any indent size (tabs, spaces) and any language.

just keep in mind that there might be better "standards".

same for lists and other complex statements:

 if (
     function1(
         parameter1,
         parameter2,
     ) &&
     function2(
         function3(
             parameter1,
             parameter2,
         ),
         parameter2,
     )
 ):
     pass

not that I have high hops... :)
Line 451: 
Line 452:     def _sysvToUpstart(self):
Line 453:         """
Line 454:         On RHEL 6, libvirtd can be started by either SysV init or Upstart.


-- 
To view, visit http://gerrit.ovirt.org/27298
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74bfe05bb4b5f5d09021f21b324f9b7d5d0fdaab
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list