[MERGE] Multiple element keys.

Robert Collins robertc at robertcollins.net
Sat Jul 28 02:10:31 BST 2007


On Fri, 2007-07-27 at 13:15 -0400, John Arbash Meinel wrote:
> 
> v- This code path could use some comments
> +            if len(elements):
> 
> v- You know how many depths you need to go, by how many elements have 
> not been
> consumed (how many are None).
> I think it would be better to make use of that, then to use 
> isinstance/type ==
> dict.

Well I thought about that, and this is within the opaque data type - it
doesn't leak to be confusing to other developers, and for this really we
should profile different approaches and find the fastest. That said I
don't expect this disk layout to last, as per my other emails. So I'm
inclined to do the simplest thing for now.

> 
> +        # XXX: To much duplication with the GraphIndex class;
> consider 
> finding
> +        # a good place to pull out the actual common logic.
> 
> ^- I was about to point it out, before I saw this. Isn't this a
> straight 
> copy
> of the other code?

Not quite. Theres some superficial differences that are easily corrected
by changing object member names. More deeply, one is to become page
based, *possibly* keeping a parsed version, and one remains memory
based. So I am thinking it needs a mixin or other form of composition to
make it clean. Until then I want the definately in-memory one clearly
separated from the 'read from disk' index.

> 
> +    def _keys_to_version_ids(self, keys):
> +        return tuple(key[0] for key in keys)
> 
> ^- Why is this returning a tuple rather than a list?
> 
> return [key[0] for key in keys]
> 
> Seems a much more natural fit.

It was a lot more natural to be a tuple before I refactored the keys to
be tuples :).

-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/20070728/e114f639/attachment.pgp 


More information about the bazaar mailing list