From: W. Trevor King Date: Tue, 21 Jul 2009 16:07:27 +0000 (-0400) Subject: Fixed extra change-hook save in testChangeHookMutableProperty. X-Git-Tag: 1.0.0~63^2~15 X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=795b15c60fd43e5d393f53926d679fb29c609359;p=be.git Fixed extra change-hook save in testChangeHookMutableProperty. The actual fix was @@ -339,7 +355,10 @@ fset = funcs.get("fset") name = funcs.get("name", "") def _fget(self, new_value=None, from_fset=False): # only used if mutable == True - value = fget(self) + if from_fset == True: + value = new_value # compare new value with cached + else: + value = fget(self) # compare current value with cached 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) The reason for the double-save was: >>> print t.settings["List-type"]==EMPTY True (the cached value here is EMPTY) >>> t.list_type = [] (old fget compares cached EMPTY to current EMPTY, no change, so no cache. fset notices change and saves EMPTY->[]) >>> t.list_type.append(5) (now fget notices the change EMPTY->[], caches [], and calls extra save) The new way: >>> print t.settings["List-type"]==EMPTY True (the cached value here is EMPTY) >>> t.list_type = [] (fget compares cached EMPTY to new [] and saves EMPTY->[]) >>> t.list_type.append(5) (fget sees no change ([]->[]), which is correct) In addition to the fix and the related corrections to testChangeHookMutableProperty, I added details about mutables to all relevant docstrings and stripped trailing whitespace from both files. --- diff --git a/libbe/properties.py b/libbe/properties.py index 8c039b2..02504e0 100644 --- a/libbe/properties.py +++ b/libbe/properties.py @@ -52,7 +52,7 @@ def Property(funcs): args["fset"] = funcs.get("fset", None) args["fdel"] = funcs.get("fdel", None) args["doc"] = funcs.get("doc", None) - + #print "Creating a property with" #for key, val in args.items(): print key, value return property(**args) @@ -77,6 +77,9 @@ def local_property(name, null=None, mutable_null=False): Define get/set access to per-parent-instance local storage. Uses .__value to store the value for a particular owner instance. If the .__value attribute does not exist, returns null. + + If mutable_null == True, we only release deepcopies of the null to + the outside world. """ def decorator(funcs): if hasattr(funcs, "__call__"): @@ -166,11 +169,16 @@ def _cmp_cached_mutable_property(self, cacher_name, property_name, value): def defaulting_property(default=None, null=None, - default_mutable=False, - null_mutable=False): + mutable_default=False): """ Define a default value for get access to a property. If the stored value is null, then default is returned. + + If mutable_default == True, we only release deepcopies of the + default to the outside world. + + null should never escape to the outside world, so don't worry + about it being a mutable. """ def decorator(funcs): if hasattr(funcs, "__call__"): @@ -181,17 +189,14 @@ def defaulting_property(default=None, null=None, def _fget(self): value = fget(self) if value == null: - if default_mutable == True: + if mutable_default == True: return copy.deepcopy(default) else: return default return value def _fset(self, value): if value == default: - if null_mutable == True: - value = copy.deepcopy(null) - else: - value = null + value = null fset(self, value) funcs["fget"] = _fget funcs["fset"] = _fset @@ -261,7 +266,7 @@ def cached_property(generator, initVal=None, mutable=False): If the input value is no longer initVal (e.g. a value has been loaded from disk or set with fset), that value overrides any cached value, and this property has no effect. - + When the cache flag is False and the stored value is initVal, the generator is not cached, but is called on every fget. @@ -270,7 +275,7 @@ def cached_property(generator, initVal=None, mutable=False): 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. + used. """ def decorator(funcs): if hasattr(funcs, "__call__"): @@ -331,6 +336,17 @@ 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. + In the case of mutables, things are slightly trickier. Because + the property-owning class has no way of knowing when the value + changes. We work around this by caching a private deepcopy of the + mutable value, and checking for changes whenever the property is + set (obviously) or retrieved (to check for external changes). So + long as you're conscientious about accessing the property after + making external modifications, mutability woln't be a problem. + t.x.append(5) # external modification + t.x # dummy access notices change and triggers hook + See testChangeHookMutableProperty for an example of the expected + behavior. """ def decorator(funcs): if hasattr(funcs, "__call__"): @@ -339,7 +355,10 @@ def change_hook_property(hook, mutable=False): fset = funcs.get("fset") name = funcs.get("name", "") def _fget(self, new_value=None, from_fset=False): # only used if mutable == True - value = fget(self) + if from_fset == True: + value = new_value # compare new value with cached + else: + value = fget(self) # compare current value with cached 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) @@ -362,7 +381,7 @@ def change_hook_property(hook, mutable=False): funcs["fset"] = _fset return funcs return decorator - + class DecoratorTests(unittest.TestCase): def testLocalDoc(self): @@ -406,7 +425,7 @@ class DecoratorTests(unittest.TestCase): @local_property(name="DEFAULT", null=5) def x(): return {} t = Test() - self.failUnless(t.x == 5, 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' diff --git a/libbe/settings_object.py b/libbe/settings_object.py index 9bc0a2f..dde247f 100644 --- a/libbe/settings_object.py +++ b/libbe/settings_object.py @@ -96,7 +96,7 @@ def versioned_property(name, doc, require_save=False): """ Combine the common decorators in a single function. - + Use zero or one (but not both) of default or generator, since a working default will keep the generator from functioning. Use the default if you know what you want the default value to be at @@ -104,22 +104,29 @@ def versioned_property(name, doc, determine a valid default at run time. If both default and generator are None, then the property will be a defaulting property which defaults to None. - + allowed and check_fn have a similar relationship, although you can use both of these if you want. allowed compares the proposed value against a list determined at 'coding time' and check_fn allows more flexible comparisons to take place at run time. - + Set require_save to True if you want to save the default/generated value for a property, to protect against future changes. E.g., we currently expect all comments to be 'text/plain' but in the future we may want to default to 'text/html'. If we don't want the old comments to be interpreted as 'text/html', we would require that the content type be saved. - + change_hook, primer, settings_properties, and required_saved_properties are only options to get their defaults into our local scope. Don't mess with them. + + Set mutable=True if: + * default is a mutable + * your generator function may return mutables + * you set change_hook and might have mutable property values + See the docstrings in libbe.properties for details on how each of + these cases are handled. """ settings_properties.append(name) if require_save == True: @@ -128,7 +135,7 @@ def versioned_property(name, doc, fulldoc = doc if default != None or generator == None: defaulting = defaulting_property(default=default, null=EMPTY, - default_mutable=mutable) + mutable_default=mutable) fulldoc += "\n\nThis property defaults to %s." % default if generator != None: cached = cached_property(generator=generator, initVal=EMPTY, @@ -180,7 +187,7 @@ class SavedSettingsObject(object): # Override. Must call ._setup_saved_settings() after loading. self.settings = {} self._setup_saved_settings() - + def _setup_saved_settings(self, flag_as_loaded=True): """ To be run after setting self.settings up from disk. Marks all @@ -208,7 +215,7 @@ class SavedSettingsObject(object): for k in self.required_saved_properties: settings[k] = getattr(self, self._setting_name_to_attr_name(k)) return settings - + def clear_cached_setting(self, setting=None): "If setting=None, clear *all* cached settings" if setting != None: @@ -392,19 +399,17 @@ class SavedSettingsObjectTests(unittest.TestCase): self.failUnless(SAVES == [ "'None' -> ''", "'' -> '[]'", - "'' -> '[]'" # <- TODO. Where did this come from? ], SAVES) self.failUnless(t.settings["List-type"] == [5],t.settings["List-type"]) self.failUnless(SAVES == [ # the append(5) has not yet been saved "'None' -> ''", "'' -> '[]'", - "'' -> '[]'", ], SAVES) self.failUnless(t.list_type == [5], t.list_type) # <-get triggers saved + self.failUnless(SAVES == [ # now the append(5) has been saved. "'None' -> ''", "'' -> '[]'", - "'' -> '[]'", "'[]' -> '[5]'" ], SAVES)