Tomas Golembiovsky has posted comments on this change. Change subject: utils: Properly handle int argument in tobool(). ......................................................................
Patch Set 4: (2 comments) https://gerrit.ovirt.org/#/c/57511/4//COMMIT_MSG Commit Message: Line 8: Line 9: The code attempts to call 'lower()' method withouth making sure the type Line 10: of the argument is a string. Line 11: Line 12: Although the code suggests the method should handle an int argument it > Why should we handle int argument in a helper for converting configuration I might have misunderstood the idea. It could be that the trick with bool(int()) is only to handle strings '0' and '1' as truth values. Line 13: actually always returns 'False' on int argument because the exception: Line 14: Line 15: AttributeError: 'int' object has no attribute 'lower' Line 16: https://gerrit.ovirt.org/#/c/57511/4/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 407 Line 408 Line 409 Line 410 Line 411 > s == string - int in not a valid input here. So if I understand it right, you would strip it to: def tobool(s): if s.lower() == 'true': return True return False What about this: def tobool(s): if not isinstance(s, basestring): raise TypeError if s.lower() == 'true': return True try: return bool(int(s)) except ValueError: return False That way we can keep the posibility to use strings '0' and '1'. -- To view, visit https://gerrit.ovirt.org/57511 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
