[MERGE] add lock hooks

Martin Pool mbp at canonical.com
Fri Apr 4 05:33:28 BST 2008


Martin Pool has voted tweak.
Status is now: Conditionally approved
Comment:
This is really nice, far better than doing it from gc.  I do have a few 
irritations with the current structure which I can't help scratching 
though.

The name and the docstring implies these are active for all physical 
locks, but they are only implemented for LockDirs, and we do currently 
also use OS locks.  I think the hook name is probably best kept generic, 
but you should at minimum document this in the dev guide, as well as in 
news.

It's unfortunate there's no namespace allowing you to reach all hooks.

+class PhysicalLockHooks(Hooks):
+    """hooks for physical lock activity."""

s//Hooks/

Shouldn't this be a private class?

+
+# The hooks instance clients should register against. Do *NOT* import 
this
+# symbol, always reference it through lock.hooks.
+hooks = PhysicalLockHooks()

It's unfortunate the current standard is an API that can so easily be 
used incorrectly.  (But, again, not in scope for this particular patch.)

+
+class LockResult(object):

Docstring?

+
+    def record_hook(self, result):
+        self._calls.append(result)
+
+    def reset_hooks(self):
+        self._old_hooks = lock.hooks
+        self.addCleanup(self.restore_hooks)
+        lock.hooks = lock.PhysicalLockHooks()
+

How about putting these on a separate class and then you can factor the 
common setup into the setUp method?

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C1207281650.5960.157.camel%40lifeless-64%3E



More information about the bazaar mailing list