Hello Zhou Zheng Sheng,
I'd like you to do a code review. Please visit
to review the following change.
Change subject: clean PEP 8 problems in vdsm/clientIF.py ......................................................................
clean PEP 8 problems in vdsm/clientIF.py
Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Signed-off-by: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com --- M Makefile.am M vdsm/clientIF.py 2 files changed, 46 insertions(+), 28 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/02/4802/1 -- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: clean PEP 8 problems in vdsm/clientIF.py ......................................................................
Patch Set 1: (3 inline comments)
.................................................... File vdsm/clientIF.py Line 76: ifrates=ifrates) How about:
self._hostStats = utils.HostStatsThread(cif=self, log=log, ifids=ifids, ifrates=ifrates)
Line 244: 'vmPayload' in drive['specParams']): I think this fits into a single line.
Line 408: continue Unrelated to your change: this code hurts my eyes ;-)
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Zhou Zheng Sheng has posted comments on this change.
Change subject: clean PEP 8 problems in vdsm/clientIF.py ......................................................................
Patch Set 1: (3 inline comments)
.................................................... File vdsm/clientIF.py Line 76: ifrates=ifrates) Thanks for the advise. I follow the instruction on indentation in PEP 8 (http://www.python.org/dev/peps/pep-0008/#indentation). It says Continuation lines should align wrapped elements.
So I can do:
self._hostStats = utils.HostStatsThread(cif=self, log=log, ifids=ifids, ifrates=ifrates)
or
self._hostStats = utils.HostStatsThread( cif=self, log=log, ifids=ifids, ifrates=ifrates)
Line 244: 'vmPayload' in drive['specParams']): If I put it into one line, it would occupy 81 characters.
Line 408: continue Well I think it can be changed into:
if f in ('vdsmd.pid', 'respawn.pid', 'svdsm.pid', 'svdsm.sock'): continue
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Zhou Zheng Sheng has posted comments on this change.
Change subject: clean PEP 8 problems in vdsm/clientIF.py ......................................................................
Patch Set 1: Verified
The changes pass vdsm test.
I examine the old version and the new version with pydiff (https://github.com/edwardbadboy/pydiff). It reports some logic changes in code, but those are all changes of 'someDict.has_key(someKey)' to 'somKey in someDict'.
pydiff report:
$ pydiff clientIF*.py 5 difference(s) first file: clientIF_old.py second file: clientIF.py
((224, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('GUID')], None, None)"), (229, "Compare(Const('GUID'), [('in', Name('drive'))])"))
((235, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('UUID')], None, None)"), (240, "Compare(Const('UUID'), [('in', Name('drive'))])"))
((238, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('specParams')], None, None)"), (243, "Compare(Const('specParams'), [('in', Name('drive'))])"))
((239, "CallFunc(Getattr(Subscript(Name('drive'), 'OP_APPLY', [Const('specParams')]), 'has_key'), [Const('vmPayload')], None, None)"), (244, "Compare(Const('vmPayload'), [('in', Subscript(Name('drive'), 'OP_APPLY', [Const('specParams')]))])"))
((251, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('path')], None, None)"), (257, "Compare(Const('path'), [('in', Name('drive'))])"))
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: clean PEP 8 problems in vdsm/clientIF.py ......................................................................
Patch Set 1: (3 inline comments)
.................................................... File vdsm/clientIF.py Line 76: ifrates=ifrates) Sounds like something we (c|sh)ould determine project wide.
Line 244: 'vmPayload' in drive['specParams']):
len(" elif 'specParams' in drive and 'vmPayload' in drive['specParams']:")
78
Line 408: continue I fully agree, but I'll submit a separate patch for this. Rather just have pep8 changes in this patch.
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Make vdsm/clientIF.py PEP8 clean ......................................................................
Patch Set 3: (1 inline comment)
.................................................... File vdsm/clientIF.py Line 253: 'vmPayload' in drive['specParams']): If you remove the parenthesis it does fit in one line.
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Shu Ming has posted comments on this change.
Change subject: Make vdsm/clientIF.py PEP8 clean ......................................................................
Patch Set 3: Verified
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Zhou Zheng Sheng has posted comments on this change.
Change subject: Make vdsm/clientIF.py PEP8 clean ......................................................................
Patch Set 3: Verified
(1 inline comment)
The patch passes vdsm tests.
pydiff report:
$ pydiff clientIF*.py 5 difference(s) first file: clientIFo.py second file: clientIF.py
((233, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('GUID')], None, None)"), (238, "Compare(Const('GUID'), [('in', Name('drive'))])"))
((244, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('UUID')], None, None)"), (249, "Compare(Const('UUID'), [('in', Name('drive'))])"))
((247, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('specParams')], None, None)"), (252, "Compare(Const('specParams'), [('in', Name('drive'))])"))
((248, "CallFunc(Getattr(Subscript(Name('drive'), 'OP_APPLY', [Const('specParams')]), 'has_key'), [Const('vmPayload')], None, None)"), (253, "Compare(Const('vmPayload'), [('in', Subscript(Name('drive'), 'OP_APPLY', [Const('specParams')]))])"))
((260, "CallFunc(Getattr(Name('drive'), 'has_key'), [Const('path')], None, None)"), (266, "Compare(Const('path'), [('in', Name('drive'))])"))
All the code logic differences are about change "aDict.has_key(aKey)" into "aKey in aDict". I think these changes are acceptable.
.................................................... File vdsm/clientIF.py Line 253: 'vmPayload' in drive['specParams']): Thanks for the advice. If I put them into one line, it will occupy 79 columns. I think the both ways are OK. If there is another person thinks I should put them into one line, I will do it.
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: Make vdsm/clientIF.py PEP8 clean ......................................................................
Patch Set 3: I would prefer that you didn't submit this
please rebase!
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Shu Ming has abandoned this change.
Change subject: Make vdsm/clientIF.py PEP8 clean ......................................................................
Patch Set 3: Abandoned
This file will be covered in the next round of pep8 patches by Zhou Zheng Sheng. And new change ID will be used for this patch.
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Shu Ming has posted comments on this change.
Change subject: Make vdsm/clientIF.py PEP8 clean ......................................................................
Patch Set 3:
See: http://gerrit.ovirt.org/#/c/5412/
-- To view, visit http://gerrit.ovirt.org/4802 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0f912dd53f84b57f09665edb80c99768006e725c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Ewoud Kohl van Wijngaarden ewoud@kohlvanwijngaarden.nl Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
vdsm-patches@lists.fedorahosted.org