Code review comment for lp:~jameinel/bzr/2.1-static-tuple-btree-string-intern

Revision history for this message
Andrew Bennetts (spiv) wrote :

384 + refs_as_tuples = tuple([tuple([tuple(ref) for ref in ref_list])
385 + for ref_list in node[3]])

I wonder if it would be worth adding a convenience method, perhaps StaticTuple.as_tuples(), that recursively does this conversion. That would make this ugliness unnecessary.

431 + # I don't believe we can define a method by which
432 + # (prefix,) + StaticTuple will work, though we could

In plain Python you could define an __radd__ for this, so surely there's a way to do this in C?

class T(object):
    def __radd__(self, other):
        return 'haha!'
t = T()

print ('tuple',) + t # prints 'haha!'

You may need to do something odd like provide the nb_add slot, even though this isn't really a numeric type, but I think that's ok. (All pure python classes would have that I think, even the non-numeric ones, so presumably having tp_as_number filled doesn't automatically make Python do dumb things.)

I think we can live without this, but it would be nice.

488 + k1 = stuple(stuple('<email address hidden>',),
489 + stuple('<email address hidden>',))
490 + k2 = stuple(stuple(stuple('<email address hidden>',),
491 + stuple('<email address hidden>',)),
492 + stuple('<email address hidden>',))

This test data is needlessly complex and hard to read. Why not e.g.:

    k1 = stuple(stuple('a',), stuple('b',))
    k2 = stuple(stuple(stuple('c',), stuple('d',)), stuple('a',))

Which is structurally the same and much easier to follow.

review: Approve

« Back to merge proposal