Nir Soffer has posted comments on this change. Change subject: janitorial: add the response.success() helper ......................................................................
Patch Set 5: (6 comments) Nice https://gerrit.ovirt.org/#/c/38270/5/lib/vdsm/response.py File lib/vdsm/response.py: Line 21: Line 22: from vdsm.define import doneCode, errCode Line 23: Line 24: Line 25: def success(**kwargs): Possible alternatives: ok(**kw)? done(**kw)? Line 26: res = {'status': doneCode} Line 27: res.update(kwargs) Line 28: return res Line 29: Line 22: from vdsm.define import doneCode, errCode Line 23: Line 24: Line 25: def success(**kwargs): Line 26: res = {'status': doneCode} This return mutable dict in doneCode - unsafe. Should create new status dict for each call. Line 27: res.update(kwargs) Line 28: return res Line 29: Line 30: Line 23: Line 24: Line 25: def success(**kwargs): Line 26: res = {'status': doneCode} Line 27: res.update(kwargs) This may override status if you do: return response.success(status="ok") So we better override kwargs with status, and not the other way around: We can do: def success(**kw): kw["status"] = {"code": doneCode["status"]["code"], "message": doneCode["status"]["code"]} return kw Which is safe as kw is always new dict. Line 28: return res Line 29: Line 30: Line 31: def error(name, message=None): https://gerrit.ovirt.org/#/c/38270/5/tests/responseTests.py File tests/responseTests.py: Line 42: Line 43: def test_success(self): Line 44: res = response.success() Line 45: self.assertEqual(res['status']['code'], 0) Line 46: self.assertEqual(res['status']['message'], 'Done') Why not self.assertEqual(res, {"status": doneCode["status"]}) Line 47: Line 48: @permutations([[{'answer': 42}], [{'fooList': ['bar', 'baz']}]]) Line 49: def test_success_with_args(self, args): Line 50: res = response.success(**args) Line 44: res = response.success() Line 45: self.assertEqual(res['status']['code'], 0) Line 46: self.assertEqual(res['status']['message'], 'Done') Line 47: Line 48: @permutations([[{'answer': 42}], [{'fooList': ['bar', 'baz']}]]) These permutations are not very useful. Line 49: def test_success_with_args(self, args): Line 50: res = response.success(**args) Line 51: self.assertEqual(res['status']['code'], 0) Line 52: self.assertEqual(res['status']['message'], 'Done') Line 50: res = response.success(**args) Line 51: self.assertEqual(res['status']['code'], 0) Line 52: self.assertEqual(res['status']['message'], 'Done') Line 53: for key in args: Line 54: self.assertEqual(res[key], args[key]) Why not res = response.success(a=1, b=2) self.assertEqual(res, {"status": doneCode["status"], "a": 1, "b": 2}) -- To view, visit https://gerrit.ovirt.org/38270 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5fcd6c832f3a16a543357570c591c1f9a907c97a Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [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
