On Wed, Aug 05, 2009 at 03:45:24PM +0100, Guido Trotter wrote: > On Wed, Aug 5, 2009 at 3:42 PM, Iustin Pop<[email protected]> wrote: > > On Wed, Aug 05, 2009 at 02:58:17PM +0100, Guido Trotter wrote: > >> > >> The method is changed to a normal loop, to avoid calling getattr() > >> twice. Also __getstate__ is changed to just use ToDict() by default. > >> > >> This should also make __getstate__ work for objects which have to > >> override the ToDict function because they contain other objects. > >> > >> __setstate__ is probably still broken in this case, but so it was > >> before, and it's not used inside our code, so I'll pretend not to have > >> noticed, as there is no "nice" way to fix it, without overriding it all > >> over the place :( > >> > >> Some unittests are added as a bonus, to make sure we behave well. > >> > >> Signed-off-by: Guido Trotter <[email protected]> > >> --- > >> Makefile.am | 1 + > >> lib/objects.py | 13 +++++----- > >> test/ganeti.objects_unittest.py | 47 > >> +++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 55 insertions(+), 6 deletions(-) > >> create mode 100755 test/ganeti.objects_unittest.py > >> > >> diff --git a/Makefile.am b/Makefile.am > >> index 875cd12..b4d3aea 100644 > >> --- a/Makefile.am > >> +++ b/Makefile.am > >> @@ -231,6 +231,7 @@ dist_TESTS = \ > >> test/ganeti.hooks_unittest.py \ > >> test/ganeti.http_unittest.py \ > >> test/ganeti.locking_unittest.py \ > >> + test/ganeti.objects_unittest.py \ > >> test/ganeti.rapi.resources_unittest.py \ > >> test/ganeti.serializer_unittest.py \ > >> test/ganeti.ssh_unittest.py \ > >> diff --git a/lib/objects.py b/lib/objects.py > >> index 1cdc98f..2cd36f4 100644 > >> --- a/lib/objects.py > >> +++ b/lib/objects.py > >> @@ -105,11 +105,7 @@ class ConfigObject(object): > >> setattr(self, key, value) > >> > >> def __getstate__(self): > >> - state = {} > >> - for name in self.__slots__: > >> - if hasattr(self, name): > >> - state[name] = getattr(self, name) > >> - return state > >> + return self.ToDict() > >> > >> def __setstate__(self, state): > >> for name in state: > >> @@ -126,7 +122,12 @@ class ConfigObject(object): > >> make sure all objects returned are only standard python types. > >> > >> """ > >> - return dict([(k, getattr(self, k, None)) for k in self.__slots__]) > >> + result = {} > >> + for name in self.__slots__: > >> + value = getattr(self, name, None) > >> + if value is not None: > >> + result[name] = value > >> + return result > > > > Please keep __getstate__ (instead of keeping ToDict), and then: > > ToDict = __getstate__ > > > > (instead of def ToDict(): return self.__getstate__) > > > > According to python's documentation __getstate__ is free to do > whatever it wants (not just a dict) as long as __setstate__ complies. > For this reason I thought it was best to keep it the other way around > (since ToDict() is clearly meant to transform into a dict). > > Also, this way classes that override ToDict() will have their > __getstate__ fixed by default, while it won't be true the other way > around.
Meh, OK, I considered __getstate__ the lower (real) implementation, whereas ToDict is a higher level. So LGTM on your original patch, but please jus say __getstate__ = ToDict instead of wrapping it. iustin
