Change models.py to use typed class members instead of a list of strings.

model.py

Index: model.py
diff --git a/model.py b/model.py
index 5f3e7da96ea95ed09c49a6d09ea82f8c55c6288b..cc26e302bff1dda917a952d1dad65aa66fc0a23d 100644
--- a/model.py
+++ b/model.py
@@ -1,27 +1,49 @@
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
-"""Defines a utility class to easily convert classes from and to dict for
-serialization.
+
+"""Defines the PersistentMixIn utility class to easily convert classes from and
+to dict for serialization.

[Peter Mayo, 2012/11/23 18:33:39]

"to and from" is usual phrase, unless you mean to emphasize "from".

[M-A Ruel, 2012/11/23 18:44:06]

done

+
+This class is aimed at json-compatible serialization, so it supports the limited
+set of structures supported by json; strings, numbers as int or float, list and
+dictionaries.
+
+PersistentMixIn._persistent_members() returns a dict of each member with the
+tuple of expected types. Each member can be decoded in multiple types, for
+example, a subversion revision number could have (None, int, str), meaning that
+the revision could be None, when not known, an int or the int as a string
+representation. The tuple is listed in the prefered order of conversions.
+
+Composites types that cannot be represented exactly in json like tuple, set and
+frozenset are converted from and back to list automatically. Any class instance
+that has been serialized can be unserialized in the same class instance or into
+a bare dict.
+
+See tests/model_tests.py for examples.
 """
 
 import json
-import sys
+import logging
 import os
 
-
+# Set in the output dict to be able to know which class was serialized to help
+# deserialization.
 TYPE_FLAG = '__persistent_type__'
-MODULE_FLAG = '__persistent_module__'
+
+# Marker to tell the deserializer that we don't know the expected type, used in
+# composite types.
+_UNKNOWN = object()
 
 
 def as_dict(value):
   """Recursively converts an object into a dictionary.
 
-  Converts tuple into list and recursively process each items.
+  Converts tuple,set,frozenset into list and recursively process each items.
   """
   if hasattr(value, 'as_dict') and callable(value.as_dict):
     return value.as_dict()
-  elif isinstance(value, (list, tuple)):
+  elif isinstance(value, (list, tuple, set, frozenset)):
     return [as_dict(v) for v in value]
   elif isinstance(value, dict):
     return dict((as_dict(k), as_dict(v))
@@ -32,23 +54,67 @@ def as_dict(value):
     raise AttributeError('Can\'t type %s into a dictionary' % type(value))
 
 
-def _inner_from_dict(value):
-  """Recursively regenerates an object."""
-  if isinstance(value, dict):
-    if TYPE_FLAG in value:
-      return PersistentMixIn.from_dict(value)
-    return dict((_inner_from_dict(k), _inner_from_dict(v))
-                for k, v in value.iteritems())
-  elif isinstance(value, list):
-    return [_inner_from_dict(v) for v in value]
-  elif isinstance(value, (float, int, basestring)) or value is None:
-    return value
+def _inner_from_dict(name, value, member_types):
+  """Recursively regenerates an object.
+
+  For each of the allowable types, try to convert it. If None is an allowable
+  type, any data that can't be parsed will be parsed as None and will be
+  silently discarded. Otherwise, an exception will be raise.
+  """
+  logging.debug('_inner_from_dict(%s, %r, %s)', name, value, member_types)
+  result = None
+  if member_types is _UNKNOWN:
+    # Use guesswork a bit more and accept anything.
+    if isinstance(value, dict) and TYPE_FLAG in value:
+      result = PersistentMixIn.from_dict(value, _UNKNOWN)
+    elif isinstance(value, list):
+      # All of these are serialized to list.
+      result = [_inner_from_dict(None, v, _UNKNOWN) for v in value]
+    elif isinstance(value, (float, int, basestring)):
+      result = value
+    else:
+      raise TypeError('No idea how to convert %r' % value)
   else:
-    raise AttributeError('Can\'t load type %s' % type(value))
+    for member_type in member_types:
+      # Explicitly leave None out of this loop.
+      if issubclass(member_type, PersistentMixIn):

[csharp, 2012/11/23 18:31:58]

Link all these high level "if member_type something" so its clear at a glance
that only one will get executed

[M-A Ruel, 2012/11/23 18:44:06]

done and used elif to make it clearer.

+        if isinstance(value, dict) and TYPE_FLAG in value:
+          result = PersistentMixIn.from_dict(value, member_type)
+          break
+
+      if member_type is dict:
+        if isinstance(value, dict):
+          result =  dict(
+              (_inner_from_dict(None, k, _UNKNOWN),
+                _inner_from_dict(None, v, _UNKNOWN))
+              for k, v in value.iteritems())
+          break
+
+      if member_type in (list, tuple, set, frozenset):
+        # All of these are serialized to list.
+        if isinstance(value, list):
+          result = member_type(
+              _inner_from_dict(None, v, _UNKNOWN) for v in value)
+          break
+
+      if member_type in (float, int, str, unicode):
+        if isinstance(value, member_type):
+          result = member_type(value)
+          break
+    else:
+      logging.info(
+          'Ignored data %r; didn\'t fit types %s',
+          value,
+          ', '.join(i.__name__ for i in member_types))
+    _check_type_value(name, result, member_types)
+  return result
 
 
 def to_yaml(obj):
-  """Converts a PersisntetMixIn into a yaml-inspired format."""
+  """Converts a PersistentMixIn into a yaml-inspired format.
+
+  Warning: Not unit tested, use at your own risk!

[csharp, 2012/11/23 18:31:58]

Why not :)

[M-A Ruel, 2012/11/23 18:44:06]

I only use it for debug output. If it becomes needed, the user will have to do
it himself.

+  """
   def align(x):
     y = x.splitlines(True)
     if len(y) > 1:
@@ -83,56 +149,131 @@ def to_yaml(obj):
   return '\n'.join(out)
 
 
+def _default_value(member_types):
+  """Returns an instance of the first allowed type. Special case None."""
+  if member_types[0] is None.__class__:
+    return None
+  else:
+    return member_types[0]()
+
+
+def _check_type_value(name, value, member_types):
+  """Raises a TypeError exception if value is not one of the allowed types in
+  member_types.
+  """
+  if not isinstance(value, member_types):
+    prefix = '%s e' % name if name else 'E'
+    raise TypeError(
+        '%sxpected type(s) %s; got %r' %
+        (prefix, ', '.join(i.__name__ for i in member_types), value))
+
+
+
 class PersistentMixIn(object):
   """Class to be used as a base class to persistent data in a simplistic way.
 
-  persistent class member needs to be set to a tuple containing the instance
-  member variable that needs to be saved or loaded.
-
-  TODO(maruel): Use __reduce__!
+  Persistent class member needs to be set to a tuple containing the instance
+  member variable that needs to be saved or loaded. The first item will be
+  default value, e.g.:
+    foo = (None, str, dict)
+  Will default initialize self.foo to None.
   """
-  persistent = None
+  # Cache of all the subclasses of PersistentMixIn.
+  __persistent_classes_cache = None
+
+  def __init__(self, **kwargs):
+    """Initializes with the default members."""
+    super(PersistentMixIn, self).__init__()
+    persistent_members = self._persistent_members()
+    for member, member_types in persistent_members.iteritems():
+      if member in kwargs:
+        value = kwargs.pop(member)
+      else:
+        value = _default_value(member_types)
+      _check_type_value(member, value, member_types)
+      setattr(self, member, value)
+    if kwargs:
+      raise AttributeError('Received unexpected initializers: %s' % kwargs)
 
-  def __new__(cls, *args, **kwargs):
-    """Override __new__() to be able to instantiate derived classes without
-    calling their __init__() function. This is useful when objects are created
-    from a dict.
+  @classmethod
+  def _persistent_members(cls):
+    """Returns the persistent items as a dict.
+
+    Each entry value can be a tuple when the member can be assigned different
+    types.
     """
-    result = super(PersistentMixIn, cls).__new__(cls)
-    if args or kwargs:
-      result.__init__(*args, **kwargs)
-    return result
+    # Note that here, cls is the subclass, not PersistentMixIn.
+    # TODO(maruel): Cache the results. It's tricky because setting
+    # cls.__persisten_members_cache on a class will implicitly set it on its

[Peter Mayo, 2012/11/23 18:33:39]

__persisten*t*_members_cache

[M-A Ruel, 2012/11/23 18:44:06]

done

+    # subclass. So in a class hierarchy with A -> B -> PersistentMixIn, calling
+    # B()._persistent_members() will incorrectly set the cache for A.
+    persistent_members_cache = {}
+    # Enumerate on the subclass, not on an instance.
+    for item in dir(cls):
+      if item.startswith('_'):
+        continue
+      item_value = getattr(cls, item)
+      if isinstance(item_value, type):
+        item_value = (item_value,)
+      if not isinstance(item_value, tuple):
+        continue
+      if not all(i is None or i.__class__ == type for i in item_value):
+        continue
+      item_value = tuple(
+          f if f is not None else None.__class__ for f in item_value)
+      persistent_members_cache[item] = item_value
+    return persistent_members_cache
+
+  @staticmethod
+  def _get_subclass(typename):
+    """Returns the PersistentMixIn subclass with the name |typename|."""
+    subclass = None
+    if PersistentMixIn.__persistent_classes_cache is not None:
+      subclass = PersistentMixIn.__persistent_classes_cache.get(typename)
+    if not subclass:
+      # Get the subclasses recursively.
+      PersistentMixIn.__persistent_classes_cache = {}
+      def recurse(c):
+        for s in c.__subclasses__():
+          assert s.__name__ not in PersistentMixIn.__persistent_classes_cache
+          PersistentMixIn.__persistent_classes_cache[s.__name__] = s
+          recurse(s)
+      recurse(PersistentMixIn)
+
+      subclass = PersistentMixIn.__persistent_classes_cache.get(typename)
+      if not subclass:
+        raise KeyError('Couldn\'t find type %s' % typename)
+    return subclass
 
   def as_dict(self):
-    """Create a dictionary out of this object."""
-    assert isinstance(self.persistent, (list, tuple))
+    """Create a dictionary out of this object, e.g. Serialize the object."""

[Peter Mayo, 2012/11/23 18:33:39]

e.g. -> i.e. ?

[M-A Ruel, 2012/11/23 18:44:06]

Right, done

     out = {}
-    for member in self.persistent:
-      assert isinstance(member, str)
-      out[member] = as_dict(getattr(self, member))
+    for member, member_types in self._persistent_members().iteritems():
+      value = getattr(self, member)
+      _check_type_value(member, value, member_types)
+      out[member] = as_dict(value)
     out[TYPE_FLAG] = self.__class__.__name__
-    out[MODULE_FLAG] = self.__class__.__module__
     return out
 
   @staticmethod
-  def from_dict(data):
+  def from_dict(data, subclass=_UNKNOWN):
     """Returns an instance of a class inheriting from PersistentMixIn,
-    initialized with 'data' dict."""
-    datatype = data[TYPE_FLAG]
-    if MODULE_FLAG in data and data[MODULE_FLAG] in sys.modules:
-      objtype = getattr(sys.modules[data[MODULE_FLAG]], datatype)
-    else:
-      # Fallback to search for the type in the loaded modules.
-      for module in sys.modules.itervalues():
-        objtype = getattr(module, datatype, None)
-        if objtype:
-          break
+    initialized with 'data' dict, e.g. Deserialize the object.
+    """
+    logging.debug('from_dict(%r, %s)', data, subclass)
+    if subclass is _UNKNOWN:
+      subclass = PersistentMixIn._get_subclass(data[TYPE_FLAG])
+    # This initializes the instance with the default values.
+    obj = subclass()
+    assert isinstance(obj, PersistentMixIn) and obj.__class__ != PersistentMixIn
+    # pylint: disable=W0212
+    for member, member_types in obj._persistent_members().iteritems():
+      if member in data:
+        value = _inner_from_dict(member, data[member], member_types)
       else:
-        raise KeyError('Couldn\'t find type %s' % datatype)
-    obj = PersistentMixIn.__new__(objtype)
-    assert isinstance(obj, PersistentMixIn)
-    for member in obj.persistent:
-      setattr(obj, member, _inner_from_dict(data.get(member, None)))
+        value = _default_value(member_types)
+      _check_type_value(member, value, member_types)
+      setattr(obj, member, value)
     return obj
 
   def __str__(self):