Code review comment for lp:~lifeless/bzr/loomsupport

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

Robert Collins wrote:
> Robert Collins has proposed merging lp:~lifeless/bzr/loomsupport into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This is an alternative implementation of lazy format objects. It seemed
> simpler to me than another custom registry subclass (needed to handle the
> network name stuff properly), and this permits slightly cleaner backwards
> compatability code in loom than otherwise.
>

+ # registered as factories.
+ if isinstance(format, MetaDirBranchFormatFactory):
+
network_format_registry.register(format.get_format_string(), format)
+ else:
+ network_format_registry.register(format.get_format_string(),
+ format.__class__)

^- This looks a bit odd.

I believe I understand why you are doing it, but it really makes me wonder.

Why are we passing in instances if what we want is factories...

I think if we add this, we should register at least a couple formats
lazily, so that people have examples to pick from.

Right now, the only example is in the test suite, and it is particularly
bad, because it uses "Class().get_format_string()" which certainly isn't
how you would use it in production. (If you had an instance of the
class, you would just register it directly...)

 review: needsfixing
 merge: approve

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

iEYEARECAAYFAkwbgMoACgkQJdeBCYSNAAN1qgCgm1mof+1ZMB20NZu+b+jTDpHn
D2QAniOCiIaL9OZ+XR0X7zWyv/ISo5sw
=k8Nv
-----END PGP SIGNATURE-----