ShaoHe Feng has uploaded a new change for review.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
fix TypeError of _eqSplit in vdsClient.py
list type can not be a hash key
Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Signed-off-by: ShaoHe Feng shaohef@linux.vnet.ibm.com --- M vdsm_cli/vdsClient.py 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/7200/1
diff --git a/vdsm_cli/vdsClient.py b/vdsm_cli/vdsClient.py index 7e02c1d..8a95dc6 100644 --- a/vdsm_cli/vdsClient.py +++ b/vdsm_cli/vdsClient.py @@ -1408,7 +1408,7 @@ if len(kv) == 2: k, v = kv else: - k, v = kv, '' + k, v = kv[0], '' d[k] = v return d
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com
Zhou Zheng Sheng has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 1: Looks good to me, but someone else must approve
If arg is 'a=b', then after "kv = arg.split('=', 1)", kv is ['a', 'b'].
If arg is 'xyz', then after "kv = arg.split('=', 1)", kv is ['xyz'].
So
k, v = kv[0], ''
is right.
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
I would prefer to drop the whole "if". Any reason not to? Clearly, the "else" branch is never used.
.................................................... File vdsm_cli/vdsClient.py Line 1407: kv = arg.split('=', 1) Line 1408: if len(kv) == 2: Line 1409: k, v = kv Line 1410: else: Line 1411: k, v = kv[0], '' it's not your fault, but why do we need this whole condition? I think it would be better to explode over sending a key with empty-string value. Line 1412: d[k] = v Line 1413: return d Line 1414: Line 1415: def _splitDriveSpecItems(self, item):
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
ShaoHe Feng has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 1: (1 inline comment)
IMO, for bool options, such as "xx=Ture" and "xx" should the same. if we drop the whole "if",it will pick up "xx=Ture" but drop the "xx".
.................................................... File vdsm_cli/vdsClient.py Line 1407: kv = arg.split('=', 1) Line 1408: if len(kv) == 2: Line 1409: k, v = kv Line 1410: else: Line 1411: k, v = kv[0], '' now I want to add new command.
I want to the command like this:
vdsClient 0 command xx=Ture
but I think
vdsClient 0 command xx
is also OK.
we can omit “=Ture”. although it is a lazy command calling. Line 1412: d[k] = v Line 1413: return d Line 1414: Line 1415: def _splitDriveSpecItems(self, item):
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: ShaoHe Feng shaohef@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: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 2:
Shao He, Dan is not against to "if...else". In fact he thinks we do not support argument written as "X=true" and "X" , we just support "X=true" for arguments. In this second patch set, you delete "if..else" and append("") to the list as a workaround to support both "X=true" and "X".
I think if we need to support both, using "if...else" is more readable and more straight forward then append("") . If we do not want to support "X", we do not need the append("") neither.
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Bing Bu Cao has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 2: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/741/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 2: I would prefer that you didn't submit this
IMO, for bool options, such as "xx=Ture" and "xx" should the same.
The patch does not give us this semantics. it makes "xx" equivalent to "xx=''", which is not really desirable.
I'd still prefer to drop handling the "xx" case, and fail such an operation. If you find it very important to have this semantics, please document it in the commit message.
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
ShaoHe Feng has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 3:
remove the 'else' branch and re-document the commit message.
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
ShaoHe Feng has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 3:
Dan, I find vdsClient.py is difficult to parser the sub-command.
I find some other lib, like python argparse module can make it ease to parser the sub-command.
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/761/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 4: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm_cli/vdsClient.py Line 1419: for arg in args: Line 1420: kv = arg.split('=', 1) Line 1421: if len(kv) == 2: Line 1422: k, v = kv Line 1423: d[k] = v Either kv = arg.split('=', 1) if len(kv) != 2: raise Exception("Unknown argument format: %s" % arg) k, v = kv d[k] = v
Or kv = arg.split('=', 1) k, v = kv d[k] = v
Or kv = arg.split('=', 1) if len(kv) == 2: "argument given as: Arg=blah" k, v = kv else: "argument given as: Arg" k, v = kv[0], '' d[k] = v
In the third case, we can write something in the commit message like "implement parsing arguments in both form 'X=Y' and just 'X'." However when I lookup the places where this function is called, I find that the caller rely on this function to report error when argument is given as "X", so I think the first two piece of code is more appropriate. Line 1424: return d Line 1425: Line 1426: def _splitDriveSpecItems(self, item): Line 1427: """
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/765/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 5: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has submitted this change and it was merged.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
fix TypeError of _eqSplit in vdsClient.py
if the arg is string 'xyz', the _eqSplit will into the 'else' branch. and then k = ['xyz']. but list type can not be a hash key. TypeError will come up.
throw a Exception to report this case, it is more clear what has happended.
Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Signed-off-by: ShaoHe Feng shaohef@linux.vnet.ibm.com --- M vdsm_cli/vdsClient.py 1 file changed, 3 insertions(+), 4 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: fix TypeError of _eqSplit in vdsClient.py ......................................................................
Patch Set 6: Verified; Looks good to me, approved
Taken, with minor changes. I hope you do not mind.
Thanks.
-- To view, visit http://gerrit.ovirt.org/7200 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I98b5573499adaa1b64f8343b445cc16df8f60f3d Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Bing Bu Cao mars@linux.vnet.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: ShaoHe Feng shaohef@linux.vnet.ibm.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Xu He Jie xuhj@linux.vnet.ibm.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org