Code review comment for lp:~jameinel/bzr/2.1-static-tuple-chk-map

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
> Looks ok, it's mostly mechanical despite the large line count. A couple of questions:
>
> 772 - self.parent_id_basename_to_file_id.key())
> 773 + (self.parent_id_basename_to_file_id.key()[0],))
>
> That change (and the other similar ones looks a bit odd.

The old code was formatting with:

"foo %s" % key

Which happened to work because 'key' was a tuple, but I don't think it
was actually *intentional*. I think it was written thinking that the
object was a string / simply formatted. I have the habit of always using
%(,) formatting, just in case the object I'm using happens to end up as
a tuple.
And the rest is just returning the simple sha1: string, rather than a
'tuple' (or StaticTuple).

I can certainly use '.as_tuple()' if you feel that is better. I've
always felt that the string formatting syntax *should* be
'str %s' % (arg1, arg2)

To make sure you don't pass an arg you don't quite understand. Sort of
the same idea that you should never do:

fprintf(file, str)

but alway

fprintf(file, "%s", str)

the former will often work, until it breaks horribly.

>
> Oh, is that how you're casting a StaticTuple to a tuple for string formatting? I would have thought "x.as_tuple()" would be about as fast as "(x[0],)", and it's certainly more readable. I suppose it is noticeably slower?
>
> 1115 -class TestSearchKeyFuncs(tests.TestCase):
>
> Why delete this TestCase?

Because all of the tests are covered in the "test__chk_map" which is
also permuted against the C and python versions of _chk_map

John
=;->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrlAgIACgkQJdeBCYSNAAMnsQCbBdRB5bnNB4CxRTjgL9Gwy5wY
sowAnjco9oE2eSg6i03FOXgmHQUdtRPa
=/cVN
-----END PGP SIGNATURE-----