Nir Soffer has posted comments on this change. Change subject: jsonrpc: ignore not needed transport ......................................................................
Patch Set 9: (1 comment) https://gerrit.ovirt.org/#/c/40173/9/lib/yajsonrpc/__init__.py File lib/yajsonrpc/__init__.py: Line 359: else: Line 360: return ("result" in obj or "error" in obj) Line 361: Line 362: def _handleMessage(self, req): Line 363: _, message = req The previous code was more clear. We know that we get a transport, and we can see that it is unused. Now see that we get unknown thing and we don't have any clue what is this _. Also on version 3, Yaniv suggested to use req[1] which is little nicer, and you said "Done", but you did not change this. I suggest to drop this patch, it does not make the code better. If you want to make the code better, change the api to use message argument instead of req tuple. If you think that sending a transport may be useful in the future or may help to debug this code, use namedtuple, so we can use: req.message Line 364: try: Line 365: mobj = json.loads(message) Line 366: isResponse = self._isResponse(mobj) Line 367: except: -- To view, visit https://gerrit.ovirt.org/40173 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icecf5c06e216151bd95f56f6fef9ed9ccba9074d Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
