[MERGE] Allow self documenting hooks.

[Martin Pool]

2009/3/10 Robert Collins <robert.collins at canonical.com>:
> On Tue, 2009-03-10 at 11:54 +1000, Martin Pool wrote:
>> +    * Hooks can now be self documenting. ``bzrlib.hooks.Hooks.create_hook``
>> +      is the entry point for this feature. (Robert Collins)
>>
>> Having different classes called Foo and Foos is a bad idea because it
>> makes it hard to talk about the two of them precisely.  That's
>> probably better handled by renaming Hooks to HookSet or something.
>
> Or we could call Hook Hookable or something like that.

HookPoint, because it makes it clearer it's the place they're hung
from, not the things that are called?

>
>> +                strings.append("An old-style hook. For documentation
>> see the __init__ "
>> +                    "method of '%s'\n" % (self.__class__.__name__,))
>>
>> Why not just insert it?
>
> Insert what?

self.__class__.__doc__ or help(self.__class__).


>
>> +    def hook(self, callback, callback_label):
>> +        """Call this hook with callback, using callback_label to describe it.
>> +
>> +        :param callback: The callable to use when this Hook fires.
>> +        :param callback_label: A label to show in the UI while this callback is
>> +            processing.
>> +        """
>> +        self._callbacks.append(callback)
>> +        self._callback_names[callback] = callback_label
>> +
>>
>> I think the docstring is wrong: this doesn't actually call the hook,
>> it rather registers the callback to be fired.
>
> Oh the first line? Sure, what about
> "Register a callback to be called when this <class> fires."

Great.

>> +        strings.append("<bzrlib.hooks.Hook(")
>>
>> It should be self.__class__.__name__ to allow it to be subclassed.
>
> This is a weak point in python really, as self.__class__.__name__ is
> 'Hook', so we need quite a bit of fudging to get the same string :(.
>
> I can do that, but I would expect folk subclassing to also change repr
> regardless - because they need to show their additional data.

I think it's still better (and also consistent with most of our other
code and docs fwiw).

Having classes with the same name that are only distinguished by their
module is of course legal but not a great idea and fairly uncommon.
We have SmartTransport not bzrlib.smart.transport.

So printing just the last component is more compact and generally not
ambiguous.  But sometimes being ambiguous if the method is not
overridden is much better than having the message actually be
incorrect.

-- 
Martin <http://launchpad.net/~mbp/>