Zhou Zheng Sheng has posted comments on this change.
Change subject: Applied PEP-8 guideline on code
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(11 inline comments)
Do you mean to submit a thorough PEP 8 clean patch? I find some problems with
vdsm/vm.py. If you want to make vdsm/vm.py PEP 8 clean, please fix those
problems and add vdsm/vm.py to PEP8_WHITELIST in Makefile.am.
# pep8 vdsm/vm.py
vdsm/vm.py:104:80: E501 line too long (95 characters)
vdsm/vm.py:205:80: E501 line too long (95 characters)
vdsm/vm.py:220:80: E501 line too long (95 characters)
vdsm/vm.py:361:80: E501 line too long (80 characters)
vdsm/vm.py:410:80: E501 line too long (81 characters)
vdsm/vm.py:508:80: E501 line too long (83 characters)
vdsm/vm.py:566:80: E501 line too long (80 characters)
vdsm/vm.py:575:80: E501 line too long (80 characters)
vdsm/vm.py:855:80: E501 line too long (80 characters)
vdsm/vm.py:977:80: E501 line too long (92 characters)
vdsm/vm.py:987:80: E501 line too long (101 characters)
vdsm/vm.py:1116:80: E501 line too long (93 characters)
vdsm/vm.py:1118:80: E501 line too long (80 characters)
vdsm/vm.py:1145:80: E501 line too long (82 characters)
....................................................
File vdsm/guestIF.py
Line 68: 'appsList': [],
Line 69: 'disksUsage': [],
Line 70: 'netIfaces': [],
Line 71: 'memoryStats': {}
Line 72: }
Does the closing brace need to stand in a single line? Can it be:
...
'netIfaces': [],
'memoryStats': {}}
Line 73: self._agentTimestamp = 0
Line 74: self._channelListener = channelListener
Line 75: if connect:
Line 76: try:
Line 205: 'session': 'Unknown',
Line 206: 'memUsage': 0,
Line 207: 'appsList': self.guestInfo['appsList'],
Line 208: 'guestIPs': self.guestInfo['guestIPs']
Line 209: }
Same as line 72.
Line 210:
Line 211: def onReboot(self):
Line 212: self.guestStatus = 'RebootInProgress'
Line 213: self.guestInfo['lastUser'] = '' + self.guestInfo['username']
....................................................
File vdsm/vm.py
Line 128: self.destServer = vdscli.connect(
Line 129: serverAddress,
Line 130: useSSL=True,
Line 131: TransportClass=kaxmlrpclib.TcpkeepSafeTransport
Line 132: )
Can I suggest:
self.destServer = vdscli.connect(
serverAddress,
useSSL=True,
TransportClass=kaxmlrpclib.TcpkeepSafeTransport)
To me, an extra indent for the parameters makes them looks different from the
"destServer", and I think the closing parenthesis does not need a whole line.
Line 133: else:
Line 134: self.destServer = kaxmlrpclib.Server('http://' +
serverAddress)
Line 135: self.log.debug('Destination server is: ' + serverAddress)
Line 136: try:
Line 253: 'dst': self._dst,
Line 254: 'mode': self._mode,
Line 255: 'method': self._method,
Line 256: 'dstparams': self._dstparams
Line 257: }
Same as line 132.
Line 258: self._vm.saveState()
Line 259: self._startUnderlyingMigration()
Line 260: self._finishSuccessfully()
Line 261: except libvirt.libvirtError, e:
Line 264: 'status': {
Line 265: 'code': errCode['migCancelErr'],
Line 266: 'message': 'Migration canceled'
Line 267: }
Line 268: }
How about the following.
self.status = {'status':
{'code': errCode['migCancelErr'],
'message': 'Migration canceled'}}
Line 269: raise
Line 270: finally:
Line 271: if '_migrationParams' in self._vm.conf:
Line 272: del self._vm.conf['_migrationParams']
Line 332: self._lastStatus = 'WaitForLaunch'
Line 333: self._migrationSourceThread =
self.MigrationSourceThreadClass(self)
Line 334: self._kvmEnable = self.conf.get('kvmEnable', 'true')
Line 335: self._guestSocketFile = constants.P_VDSM_RUN +
self.conf['vmId'] + \
Line 336: '.guest.socket'
The original indentation looks more readable to me.
Line 337: self._incomingMigrationFinished = threading.Event()
Line 338: self.id = self.conf['vmId']
Line 339: self._volPrepareLock = threading.Lock()
Line 340: self._initTimePauseCode = None
Line 394: res = self.cif.irs.getVolumeSize(
Line 395: drv['domainID'],
Line 396: drv['poolID'],
Line 397: drv['imageID'],
Line 398: drv['volumeID'])
A little more indentation could make it more readable.
res = self.cif.irs.getVolumeSize(
drv['domainID'],
drv['poolID'],
drv['imageID'],
drv['volumeID'])
Line 399: drv['truesize'] = res['truesize']
Line 400: drv['apparentsize'] = res['apparentsize']
Line 401: else:
Line 402: drv['truesize'] = 0
Line 494: 'device': 'memballoon',
Line 495: 'specParams': {
Line 496: 'model': 'none'
Line 497: }
Line 498: })
Since indentation has already given the information of the syntax level of the
elements, I don't like single lines contains only braces and parentheses.
Line 499:
Line 500: return devices
Line 501:
Line 502: def getConfController(self):
Line 798: if newSize is None:
Line 799: # newSize is always in megabytes
Line 800: newSize = (config.getint('irs',
'volume_utilization_chunk_mb')
Line 801: + ((vmDrive.apparentsize + constants.MEGAB - 1)
Line 802: / constants.MEGAB))
It's better to split the line after a binary operator as follow.
newSize = (config.getint('irs', 'volume_utilization_chunk_mb') +
((vmDrive.apparentsize + constants.MEGAB - 1) /
constants.MEGAB))
Line 803:
Line 804: if getattr(vmDrive, 'diskReplicate', None):
Line 805: volInfo = {'poolID': vmDrive.diskReplicate['poolID'],
Line 806: 'domainID': vmDrive.diskReplicate['domainID'],
Line 852:
Line 853: if volSizeRes['status']['code']:
Line 854: raise RuntimeError(
Line 855: "Cannot get the volume size for %s (domainID: %s,
volumeID: %s)"
Line 856: % (volInfo['name'],
It's better to split the line after a binary operator. For example,
raise RuntimeError(
"Cannot get the volume size for "
"%s (domainID: %s, volumeID: %s)" %
(volInfo['name'], volInfo['domainID'], volInfo['volumeID']))
Line 857: volInfo['domainID'],
Line 858: volInfo['volumeID'])
Line 859: )
Line 860:
Line 1087: 'kvmEnable': self._kvmEnable,
Line 1088: 'network': {}, 'disks': {},
Line 1089: 'monitorResponse': str(self._monitorResponse),
Line 1090: 'elapsedTime': str(int(time.time() - self._startTime)),
Line 1091: }
The original style is OK. Need not to change.
Line 1092: if 'cdrom' in self.conf:
Line 1093: stats['cdrom'] = self.conf['cdrom']
Line 1094: if 'boot' in self.conf:
Line 1095: stats['boot'] = self.conf['boot']
--
To view, visit http://gerrit.ovirt.org/8933
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b1ce77b01d848adb008d375decb2c214617ad2e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Gal Hammer <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Peter V. Saveliev <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
