Nir Soffer has posted comments on this change. Change subject: properties: Add properties module ......................................................................
Patch Set 6: (8 comments) https://gerrit.ovirt.org/#/c/40822/6/lib/vdsm/properties.py File lib/vdsm/properties.py: Line 119: if not required and default not in values: Line 120: raise ValueError("Default value %s not in allowed values %s" % Line 121: (default, values)) Line 122: super(Enum, self).__init__(required=required, default=default, doc=doc) Line 123: self.values = values Check that all values of of the same type. Line 124: Line 125: def validate(self, value): Line 126: if value not in self.values: Line 127: raise ValueError("Invalid value %r for property %s" % Line 172: Line 173: class Float(_Number): Line 174: Line 175: def validate(self, value): Line 176: if not isinstance(value, float): Should accept also integers: if not isinstance(value, (float, int)): ... Line 177: raise ValueError("Invalid value %r for float property %s" % Line 178: (value, self.name)) Line 179: return super(Float, self).validate(value) Line 180: https://gerrit.ovirt.org/#/c/40822/6/tests/propertiesTests.py File tests/propertiesTests.py: Line 74: self.value = value Line 75: Line 76: def test_required(self): Line 77: # Required property set to None is considered unset Line 78: self.assertRaises(ValueError, self.Cls) > Should we add a test like so? Probably yes. Line 79: Line 80: Line 81: class PropertyDefaultTests(VdsmTestCase): Line 82: Line 112: obj = self.Cls() Line 113: self.assertEqual(obj.value, "2") Line 114: Line 115: def test_allowed(self): Line 116: for value in ("1", "2", "3"): > Can an enum have values of mixed types (eg. [1, "2", u"3"]) ? I know it's I guess we cannot and should not support this. We need more tests. Line 117: obj = self.Cls() Line 118: obj.value = value Line 119: self.assertEqual(obj.value, value) Line 120: Line 121: def test_forbidden(self): Line 122: obj = self.Cls() Line 123: self.assertRaises(ValueError, setattr, obj, "value", "4") Line 124: Line 125: def test_none(self): > So None is always allowed even if it was not specified in the values list? I think I changed it to make it easier to handle missing value in a dict as unset value: class Foo: value = properties.Integer() def __init__(self, d): self.value = d.get("foo") You don't want to do: if "foo" in d: self.value = d["foo"] Properties exists to prevent this boilerplate. Line 126: obj = self.Cls() Line 127: obj.value = None Line 128: self.assertEqual(None, obj.value) Line 129: Line 270: self.assertEqual(obj.value, 3.14) Line 271: Line 272: def test_invalid(self): Line 273: obj = self.Cls() Line 274: self.assertRaises(ValueError, setattr, obj, "value", "not a float") > Are integers allowed? ie. 3 ? It would be nice to allow 3 without needing No, I check for float. I'm not big fan of type automatic type conversion, but treating integers as floats should be safe and should be added. Need to add a test. Line 275: Line 276: def test_none(self): Line 277: obj = self.Cls() Line 278: obj.value = None Line 290: self.assertEqual(1.0, obj.value) Line 291: Line 292: def test_too_small(self): Line 293: obj = self.Cls() Line 294: self.assertRaises(ValueError, setattr, obj, "value", -1.0) > You might want to test with a value that is really, really close to the min Aren't < or > good enough for comparing floats? See the _Number class in properties.py Line 295: Line 296: Line 297: class FloatMaxValueTests(VdsmTestCase): Line 298: Line 350: Line 351: def test_none(self): Line 352: obj = self.Cls() Line 353: obj.value = None Line 354: self.assertEqual(None, obj.value) > Test with 0, 1, [], {}, and other costructs that are implicitly interpreted Anything which is not True or False will raise ValueError, see test_invalid. But yes, we need more tests, maybe with permutations. Line 355: Line 356: Line 357: class BooleanDefaultTests(VdsmTestCase): Line 358: -- To view, visit https://gerrit.ovirt.org/40822 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I62175172dddce7f84319e3c8669747994e06a697 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@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