Francesco Romani has posted comments on this change. Change subject: lib: migration: add response helpers ......................................................................
Patch Set 4: (2 comments) https://gerrit.ovirt.org/#/c/42795/4/lib/vdsm/response.py File lib/vdsm/response.py: Line 23: from vdsm.define import doneCode Line 24: from vdsm.define import errCode Line 25: Line 26: Line 27: class Malformed(Exception): > I prefer MalformedError... But if you like it this way - fine. I was thinking myself and not too happily settled on Malformed because response.Malformed reads a tiny bit better than response.MalformedError but it is a silly reason, and since you want MalformedError, I see no reason not to keep it this way. Line 28: """ Line 29: Malformed response value Line 30: """ Line 31: Line 59: Line 60: def is_success(res): Line 61: try: Line 62: code = res["status"]["code"] Line 63: except KeyError: > I don't find this exception translation helpful, but again - let it be. Again, I was not terribly happy either. But here I have a bit stronger reasons, because here KeyError here seems a layering violation (big name but simple concept). The whole point of having response module is to go have a higher level of abstraction than juggling with nested dicts: we want to reason in terms of response values. So here the problem is that one Response is Malformed, which in turn is caused by a missing Key (hence the KeyError) - but I see the latter as an implementation detail (so one layer below). Thus, I believe that this is one of the few cases on which the exception translation makes sense. But we can meet halfway with this: def is_success(res): if "status" not in res or "code" not in res["status"]: raise MalformedError return res["status"]["code"] == doneCode["code"] what do you think? Line 64: raise Malformed Line 65: else: Line 66: return code == doneCode["code"] Line 67: -- To view, visit https://gerrit.ovirt.org/42795 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ccefec4f1bebcb2ca64f0bc9f6b9e9954dbf04c Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Marcin Mirecki <mmire...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
