Yaniv Bronhaim has posted comments on this change.

Change subject: xmlrpc: Parsing error logging enhancement - vdsClient
......................................................................


Patch Set 4:

(2 comments)

....................................................
File lib/vdsm/vdscli.py.in
Line 40: 
Line 41:     def __getattr__(self, name):
Line 42:         if hasattr(self._transport, name):
Line 43:             func = getattr(self._transport, name)
Line 44:             if name == 'parse_response':
do you wrap only one function called parse_response?? so modify only the 
specific function.. why do you need it for each call
Line 45:                 return lambda *args, **kwargs: self._wrap(name,
Line 46:                                                           func, args, 
kwargs)
Line 47:             else:
Line 48:                 return func


Line 57:         except ExpatError as e:
Line 58:             sys.stderr.write('Parsing error was thrown during parsing '
Line 59:                              'response. Following arguments were 
passed: ')
Line 60:             for arg in enumerate(args):
Line 61:                 sys.stdout.write(arg[1])
you need to specify "\n" in this usage btw.. not sure if here its needed.

and why did you change this part ? can you add comment what should be settled 
in arg[1] in this case?
Line 62:             raise e
Line 63:         return result
Line 64: 
Line 65: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife29c4f7749b9cd8a4ad892f486d91509e505ae4
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: mooli tayer <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to