[MERGE] FormatRegistry

[Aaron Bentley]

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

John Arbash Meinel wrote:
> Aaron Bentley wrote:
> +class BzrDirFormatRegistry(registry.Registry):
> +    """Registry of user-selectable BzrDir subformats.
> +
> +    Differs from BzrDirFormat._control_formats in that it provides
> sub-formats,
> +    e.g. BzrDirMeta1 with weave repository.  Also, it's more user-oriented.
> +    """
> +
> +    def register_metadir(self, key, repo, help, native=True,
> deprecated=False):


> ^- It seems very restrictive to force all repository formats to be in
> 'bzrlib.repository'. I think it would be good to give the python module
> and path, since that lets us start to split things up.

This is a helper to make the common case easy.  Anyone can use a
repository from anywhere.  They just have to use register_factory
(actually, now it's 'register').

> v- Why explictly disable register_lazy?

As I understood it, the target of the lazy-load was required to be a
callable whose first parameter was the key.  So I felt it was
unsuitable, because BzrDir constructors don't match that signature.  I'm
seeing now that this restriction is actually on the optional help callable.

As a result, I'm zapping register_factory, and using just register for
that purpose.  And I'm restoring register_lazy, because I see that it
can work, now.

I'm also not a big fan of throwing strings around when you really mean
Python identifiers.  So while register_metadir seemed useful,
register_lazy seems overboard.  But it's there, and tested and everything.

> I could understand making a
> different helper for it, but part of the point of Registry is so that
> people don't *have* to write so many helpers, especially for lazy loading.

I would much rather write a two-line helper.

> v- Is there a reason you chose to use the key 'default', rather than
> just using Registry.default = 'knit' ?

Yes.  The existing code treats "default" as if it's just another format,
on par with "knit".  So I wanted get("default") to work, and I wanted
keys() to include "default", and iteritems(), etc., etc.

> +def get_format_topic(topic):
> +    import bzrdir
> +    return bzrdir.format_registry.help_topic(topic)
> +topic_registry.register('formats', get_format_topic, 'directory formats')
> 
> ^- Like here, instead of creating a wrapper which has to import
> something, you could just do:
> 
> topic_registry.register_lazy('formats', 'bzrlib.bzrdir',
> 			     'format_registry.help_topic',
> 			      'directory formats')
> 
> Though since you are nested 2 deep, that might need:

I *know* how to create a lazy wrapper.  Can do it in my sleep.

register_lazy freaks me, because I can tell it's not import.  Which
means I have to learn something else that's not import.  So considering
how little it does, I think using register_lazy is a net loss.

> At the very least, though, you should be doing an absolute import. Either:

Sure.

> v-  You should be using 'assertIsInstance()' rather than
> assertTrue(isinstance)

Oh, didn't realize we had one.

> Otherwise, I think it looks pretty good. Though I might prefer it if we
> switched the actual BzrDir._control_formats list, rather than having
> *another* registry just for a help topic.

I certainly considered that option.  But my conclusion was that they are
two different lists.  If it was simply about attaching help text, I
certainly would have worked on turning BzrDir._control_formats into a
registry, instead.

But BzrDir._control_formats is not intended as a list of subformats.  I
think it would do a very poor job as one.

> It seems like having 2 lists is going to make it easy for them to get
> out of sync.

They're already out of sync and they're supposed to be out of sync.
There's some overlap, but _control_formats only contains BzrDirs, and
doesn't require them to be writable formats.  So it can safely contain
BzrDirFormat4 or GitBzrDir.  It's not at all concerned about repository,
working tree and branch formats -- whatever's there is there.

But on the flip side, _control_formats is not specific enough for
creating new repos/branches/trees from scratch, because metadirs are so
flexible.  There's no specific BzrDir format for 'metaweave', 'knit' and
'experimental-knit2', and such a restriction would be artificial.

And if you were to instantiate http://bazaar-vcs.org/bzr/bzr.dev from
format_registry, you've got three matches: "metaweave", "knit", and
"experimental-knit2" all use have the same "branch-format" file.  It
doesn't make sense to try and decide between them, because for
instantiation purposes, they're the same.

So the _control_formats list is too general for creating .bzr
directories, and format_registry is too specific for probing purposes.

This way, by registering subformats, there's a good combination of
flexability and safety.  There's no chance of getting a bad
combination-- there's no syntax you can use to create a tree with a
knit2 repository and a workingtree2 tree.  (Unless you register such a
combo as valid).

So I really think that the split is justified.

Here's an updated bundle.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFig+/0F+nu1YWqI0RAj0jAJwM4lhHBr1N7XYfY61CB3Aa4ng0WQCfSCRE
6dKBavTKYi5kGmqLdtKCkV0=
=JhJE
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: format-registry2.patch
Type: text/x-patch
Size: 43249 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061220/aaa8c70e/attachment.bin