[PATCH] Factor out duplicate code

[Robert Collins]

On Wed, 2005-10-12 at 17:59 -0500, John Arbash Meinel wrote:

> @staticmethod
> def create(kind, file_id, *args, **kwargs):
>   try:
>     return InventoryEntry.versionable_kinds[kind](file_id,
> 	*args, **kwargs)
>   except ???
> 
> Then you still have the dictionary to make it useful. And if the kind
> doesn't take all the arguments given to it, then an exception will be
> raised.

Several reasons. *args, **kwargs methods are incredibly opaque. Theres
generally only one reason to have them anywhere, and that is when you
have callable decorators - such as 'apply_redirected'. Anything that can
know what it is doing should know what it is doing.

Also, consider the uses for creating from 'kind' - there are two:
- we have a 'set_inventory' method on branch that takes a list of
parameters to create entries from. Preserving that interface in the
short term is useful, and, perhaps, in the long term. Related to this is
XML deserialisation, but it creates concrete types.
- we want to create an entry from a random file on disk.
For the former, if we want a sensible error, we need to check the list
of parameters against the type. If we let python do that, we get a
TypeError, which is, while accurate, not that helpful - callers cannot
tell that they got something wrong vs us having a bug. If we do it
ourselves we can be very precise about the error 'Directories do not
have hash values'. For the latter, we want an interface that takes a
real file's stat information, chooses the right type of entry to create
and does that. This might be via a class method on each class - a
variation on __init__ if you like, that will take a path and stat
information and do whatever is needed to get a fully fledged entry.

> The idea of the dictionary is to let the information about creating an
> inventory entry be distributed, so each individual entry knows how to
> create itself, rather than having InventoryEntry.create() have to know
> in advance about all types that might be created.

MMM. Thats not possible with the current interface, because to know how
to create a complete InventoryLink, I need a tree that supports
get_file_symlink. Add to that that one of the points of factories is to
encapsulate any gymnastics involved in created a FOO from some non-FOO
set of parameters, and it seems to me that dictionary harms things - it
pushes inappropriate logic into the domain class, which makes it less
flexable and ties it to a specific creation scenario.

> I really think a dictionary should almost always be used for factories.

I don't.

Rob


-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20051013/df203fd7/attachment.pgp