tag --remove now returns bug.settings["extra_strings"] to EMPTY.

extra_strings returns to a defaulting property from a cached/generator
property, with the help of the new, mutable defaults.  Lots of
deepcopies avoid mutable default uncertainty too ;).  And
copy.deepcopy([]) should be pretty cheap.

tag --remove had previously left settings["extra_strings"] as [],
which polluted the bug's values file.  Now the improved
defaulting_property notices a return to the default [], and sets the
internally stored value to EMPTY.

I struggled with creating a more intuitive way to notice changes to
extra_strings than the
  tmp = bug.extra_strings
  <work on tmp>
  bug.extra_strings = tmp
but didn't have any luck.  The problem seems to be that if you only
hand out copies of your default, you don't have any pointers to what
you handed out to check for changes.  On the other hand, if you hand
out your original default, any external changes will _change_ your
original default.  I suppose you could only hand out copies, but keep
a list of all copies handed out, but that sounds like a disaster.
Reassigning is easy enough.

4 files changed, 97 insertions, 72 deletions

diff --git a/becommands/tag.py b/becommands/tag.py
index 0b19e91..b8030f3 100644
--- a/becommands/tag.py
+++ b/becommands/tag.py
@@ -61,6 +61,11 @@ def execute(args, test=False):
     >>> a = bd.bug_from_shortname("a")
     >>> print a.extra_strings
     []
+    >>> execute(["a", "Alphabetically first"], test=True)
+    Tagging bug a:
+    Alphabetically first
+    >>> execute(["--remove", "a", "Alphabetically first"], test=True)
+    Tags for a:
     """
     parser = get_parser()
     options, args = parser.parse_args(args)
@@ -79,7 +84,6 @@ def execute(args, test=False):
     new_tag = None
     if len(args) == 2:
         given_tag = args[1]
-        # reassign list so the change_hook realizes we've altered it.
         tags = bug.extra_strings
         tag_string = "TAG:%s" % given_tag
         if options.remove == True:
@@ -87,15 +91,15 @@ def execute(args, test=False):
         else: # add the tag
             new_tag = given_tag
             tags.append(tag_string)
-        bug.extra_strings = tags
+        bug.extra_strings = tags # reassign to notice change
+
+    bug.save()
 
     tags = []
     for estr in bug.extra_strings:
         if estr.startswith("TAG:"):
             tags.append(estr[4:])
 
-    bd.save()
-
     if new_tag == None:
         print "Tags for %s:" % bug.uuid
     else:
diff --git a/libbe/bug.py b/libbe/bug.py
index 0f912cf..7418933 100644
--- a/libbe/bug.py
+++ b/libbe/bug.py
@@ -185,11 +185,11 @@ class Bug(settings_object.SavedSettingsObject):
                     fset=_set_time,
                     doc="An integer version of .time_string")
 
-    def _extra_strings_generator(self):
-        return []
     def _extra_strings_check_fn(value):
         "Require an iterable full of strings"
-        if not hasattr(value, "__iter__"):
+        if value == settings_object.EMPTY:
+            return True
+        elif not hasattr(value, "__iter__"):
             return False
         for x in value:
             if type(x) not in types.StringTypes:
@@ -200,7 +200,7 @@ class Bug(settings_object.SavedSettingsObject):
         self._prop_save_settings(old, new)
     @_versioned_property(name="extra_strings",
                          doc="Space for an array of extra strings.  Useful for storing state for functionality implemented purely in becommands/<some_function>.py.",
-                         generator=_extra_strings_generator,
+                         default=[],
                          check_fn=_extra_strings_check_fn,
                          change_hook=_extra_strings_change_hook,
                          mutable=True)
diff --git a/libbe/properties.py b/libbe/properties.py
index 2ccc50b..956ecc3 100644
--- a/libbe/properties.py
+++ b/libbe/properties.py
@@ -71,7 +71,7 @@ def doc_property(doc=None):
         return funcs
     return decorator
 
-def local_property(name, null=None):
+def local_property(name, null=None, mutable_null=False):
     """
     Define get/set access to per-parent-instance local storage.  Uses
     ._<name>_value to store the value for a particular owner instance.
@@ -85,7 +85,11 @@ def local_property(name, null=None):
         def _fget(self):
             if fget is not None:
                 fget(self)
-            value = getattr(self, "_%s_value" % name, null)
+            if mutable_null == True:
+                ret_null = copy.deepcopy(null)
+            else:
+                ret_null = null
+            value = getattr(self, "_%s_value" % name, ret_null)
             return value
         def _fset(self, value):
             setattr(self, "_%s_value" % name, value)
@@ -123,7 +127,46 @@ def settings_property(name, null=None):
         return funcs
     return decorator
 
-def defaulting_property(default=None, null=None):
+
+# Allow comparison and caching with _original_ values for mutables,
+# since
+#
+# >>> a = []
+# >>> b = a
+# >>> b.append(1)
+# >>> a
+# [1]
+# >>> a==b
+# True
+def _hash_mutable_value(value):
+    return repr(value)
+def _init_mutable_property_cache(self):
+    if not hasattr(self, "_mutable_property_cache_hash"):
+        # first call to _fget for any mutable property
+        self._mutable_property_cache_hash = {}
+        self._mutable_property_cache_copy = {}
+def _set_cached_mutable_property(self, cacher_name, property_name, value):
+    _init_mutable_property_cache(self)
+    self._mutable_property_cache_hash[(cacher_name, property_name)] = \
+        _hash_mutable_value(value)
+    self._mutable_property_cache_copy[(cacher_name, property_name)] = \
+        copy.deepcopy(value)
+def _get_cached_mutable_property(self, cacher_name, property_name, default=None):
+    _init_mutable_property_cache(self)
+    if (cacher_name, property_name) not in self._mutable_property_cache_copy:
+        return default
+    return self._mutable_property_cache_copy[(cacher_name, property_name)]
+def _cmp_cached_mutable_property(self, cacher_name, property_name, value):
+    _init_mutable_property_cache(self)
+    if (cacher_name, property_name) not in self._mutable_property_cache_hash:
+        return 1 # any value > non-existant old hash
+    old_hash = self._mutable_property_cache_hash[(cacher_name, property_name)]
+    return cmp(_hash_mutable_value(value), old_hash)
+
+
+def defaulting_property(default=None, null=None,
+                        default_mutable=False,
+                        null_mutable=False):
     """
     Define a default value for get access to a property.
     If the stored value is null, then default is returned.
@@ -133,14 +176,21 @@ def defaulting_property(default=None, null=None):
             funcs = funcs()
         fget = funcs.get("fget")
         fset = funcs.get("fset")
+        name = funcs.get("name", "<unknown>")
         def _fget(self):
             value = fget(self)
             if value == null:
-                return default
+                if default_mutable == True:
+                    return copy.deepcopy(default)
+                else:
+                    return default
             return value
         def _fset(self, value):
             if value == default:
-                value = null
+                if null_mutable == True:
+                    value = copy.deepcopy(null)
+                else:
+                    value = null
             fset(self, value)
         funcs["fget"] = _fget
         funcs["fset"] = _fset
@@ -195,7 +245,7 @@ def checked_property(allowed=[]):
         return funcs
     return decorator
 
-def cached_property(generator, initVal=None):
+def cached_property(generator, initVal=None, mutable=False):
     """
     Allow caching of values generated by generator(instance), where
     instance is the instance to which this property belongs.  Uses
@@ -204,7 +254,7 @@ def cached_property(generator, initVal=None):
 
     When the cache flag is True or missing and the stored value is
     initVal, the first fget call triggers the generator function,
-    whiose output is stored in _<name>_cached_value.  That and
+    whose output is stored in _<name>_cached_value.  That and
     subsequent calls to fget will return this cached value.
 
     If the input value is no longer initVal (e.g. a value has been
@@ -216,25 +266,27 @@ def cached_property(generator, initVal=None):
 
     The cache flag is missing on initialization.  Particular instances
     may override by setting their own flag.
+
+    In the case that mutable == True, all caching is disabled and the
+    generator is called whenever the cached value would otherwise be
+    used.  This avoids uncertainties in the value of stored mutables.
     """
     def decorator(funcs):
         if hasattr(funcs, "__call__"):
             funcs = funcs()
         fget = funcs.get("fget")
-        fset = funcs.get("fset")
         name = funcs.get("name", "<unknown>")
         def _fget(self):
             cache = getattr(self, "_%s_cache" % name, True)
             value = fget(self)
-            if cache == True:
-                if value == initVal:
+            if value == initVal:
+                if cache == True and mutable == False:
                     if hasattr(self, "_%s_cached_value" % name):
                         value = getattr(self, "_%s_cached_value" % name)
                     else:
                         value = generator(self)
                         setattr(self, "_%s_cached_value" % name, value)
-            else:
-                if value == initVal:
+                else:
                     value = generator(self)
             return value
         funcs["fget"] = _fget
@@ -278,20 +330,6 @@ def change_hook_property(hook, mutable=False):
     called _after_ the new value has been stored, allowing you to
     change the stored value if you want.
 
-    If mutable=True, store a string-representation of the old_value
-    for use in comparisions, since
-
-    >>> a = []
-    >>> b = a
-    >>> b.append(1)
-    >>> a
-    [1]
-    >>> a==b
-    True
-
-    The string-value-changed test may miss the first write, since
-    there will not have been an opportunity to cache a string version
-    of the old value.
     """
     def decorator(funcs):
         if hasattr(funcs, "__call__"):
@@ -299,37 +337,16 @@ def change_hook_property(hook, mutable=False):
         fget = funcs.get("fget")
         fset = funcs.get("fset")
         name = funcs.get("name", "<unknown>")
-        def hash_value(value): # only used if mutable == True
-            return repr(value)
         def _fget(self, new_value=None, from_fset=False): # only used if mutable == True
             value = fget(self)
-            if not hasattr(self, "_change_hook_property_mutable_cache_hash"):
-                # first call to _fget for any mutable property
-                self._change_hook_property_mutable_cache_hash = {}
-                self._change_hook_property_mutable_cache_copy = {}
-            if name not in self._change_hook_property_mutable_cache_hash:
-                # first call to _fget for this particular mutable property
-                self._change_hook_property_mutable_cache_hash[name] = \
-                    hash_value(new_value)
-                self._change_hook_property_mutable_cache_copy[name] = \
-                    copy.deepcopy(new_value)
-            elif from_fset == True: # return cached value, and cache new value
-                old_hash = self._change_hook_property_mutable_cache_hash[name]
-                if hash_value(value) != old_hash:
-                    value = self._change_hook_property_mutable_cache_copy[name]
-                    self._change_hook_property_mutable_cache_hash[name] = \
-                        hash_value(new_value)
-                    self._change_hook_property_mutable_cache_copy[name] = \
-                        copy.deepcopy(new_value)
-            else: # check for a change in value while we weren't looking
-                old_hash = self._change_hook_property_mutable_cache_hash[name]
-                if hash_value(value) != old_hash:
-                    old_val=self._change_hook_property_mutable_cache_copy[name]
-                    self._change_hook_property_mutable_cache_hash[name] = \
-                        hash_value(value)
-                    self._change_hook_property_mutable_cache_copy[name] = \
-                        copy.deepcopy(value)
-                    hook(self, old_val, value)
+            if _cmp_cached_mutable_property(self, "change hook property", name, value) != 0:
+                # there has been a change, cache new value
+                old_value = _get_cached_mutable_property(self, "change hook property", name)
+                _set_cached_mutable_property(self, "change hook property", name, value)
+                if from_fset == True: # return previously cached value
+                    value = old_value
+                else: # the value changed while we weren't looking
+                    hook(self, old_value, value)
             return value
         def _fset(self, value):
             if mutable == True: # get cached previous value
@@ -344,7 +361,7 @@ def change_hook_property(hook, mutable=False):
         funcs["fset"] = _fset
         return funcs
     return decorator
-
+        
 
 class DecoratorTests(unittest.TestCase):
     def testLocalDoc(self):
@@ -385,16 +402,18 @@ class DecoratorTests(unittest.TestCase):
         class Test(object):
             @Property
             @defaulting_property(default='y', null='x')
-            @local_property(name="DEFAULT")
+            @local_property(name="DEFAULT", null=5)
             def x(): return {}
         t = Test()
-        self.failUnless(t.x == None, str(t.x)) 
+        self.failUnless(t.x == 5, str(t.x)) 
         t.x = 'x'
         self.failUnless(t.x == 'y', str(t.x))
         t.x = 'y'
         self.failUnless(t.x == 'y', str(t.x))
         t.x = 'z'
         self.failUnless(t.x == 'z', str(t.x))
+        t.x = 5
+        self.failUnless(t.x == 5, str(t.x))
     def testCheckedLocalProperty(self):
         class Test(object):
             @Property
@@ -574,21 +593,21 @@ class DecoratorTests(unittest.TestCase):
         a = t.x
         self.failUnless(t.old == [], t.old)
         self.failUnless(t.new == [5], t.new)
-        self.failUnless(t.hook_calls == 5, t.hook_calls)
+        self.failUnless(t.hook_calls == 6, t.hook_calls)
         t.x.append(6) # this append(6) is not noticed yet
         self.failUnless(t.old == [], t.old)
         self.failUnless(t.new == [5,6], t.new)
-        self.failUnless(t.hook_calls == 5, t.hook_calls)
+        self.failUnless(t.hook_calls == 6, t.hook_calls)
         # this append(7) is not noticed, but the t.x get causes the
         # append(6) to be noticed
         t.x.append(7)
         self.failUnless(t.old == [5], t.old)
         self.failUnless(t.new == [5,6,7], t.new)
-        self.failUnless(t.hook_calls == 6, t.hook_calls)
+        self.failUnless(t.hook_calls == 7, t.hook_calls)
         a = t.x # now the append(7) is noticed
         self.failUnless(t.old == [5,6], t.old)
         self.failUnless(t.new == [5,6,7], t.new)
-        self.failUnless(t.hook_calls == 7, t.hook_calls)
+        self.failUnless(t.hook_calls == 8, t.hook_calls)
 
 
 suite = unittest.TestLoader().loadTestsFromTestCase(DecoratorTests)
diff --git a/libbe/settings_object.py b/libbe/settings_object.py
index a854ce5..60fddb9 100644
--- a/libbe/settings_object.py
+++ b/libbe/settings_object.py
@@ -126,10 +126,12 @@ def versioned_property(name, doc,
     def decorator(funcs):
         fulldoc = doc
         if default != None:
-            defaulting  = defaulting_property(default=default, null=EMPTY)
+            defaulting  = defaulting_property(default=default, null=EMPTY,
+                                              default_mutable=mutable)
             fulldoc += "\n\nThis property defaults to %s" % default
         if generator != None:
-            cached = cached_property(generator=generator, initVal=EMPTY)
+            cached = cached_property(generator=generator, initVal=EMPTY,
+                                     mutable=mutable)
             fulldoc += "\n\nThis property is generated with %s" % generator
         if check_fn != None:
             fn_checked = fn_checked_property(value_allowed_fn=check_fn)