Change in vdsm[master]: properties: Add properties module
nsoffer at redhat.com
nsoffer at redhat.com
Thu Dec 10 23:07:57 UTC 2015
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 <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list