Code review comment for lp://staging/~spiv/bzr-loom/plugin-init

On Fri, Aug 13, 2010 at 7:13 PM, Andrew Bennetts
<email address hidden> wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr-loom/plugin-init into lp:bzr-loom.
>
> Requested reviews:
>  Loom Developers (bzr-loom-devs)
> Related bugs:
>  #617212 "bzr selftest -s bt.per_interbranch Loom" finds 0 tests
>  https://bugs.launchpad.net/bugs/617212
>
>
> This branch does a couple of loosely related things:
>  * makes sure InterLoomBranch is always registered, by moving its definition to formats.py.  I don't think this is a great solution, but it does solve the problem.  Certainly by the time formats.register.formats() has been called that InterLoomBranch ought to be installed one way or another I think.  This fixes bug 617212.

Probably ok.

>  * replaces "import bzrlib.branch" etc with "from bzrlib import branch" etc, following bzrlib's own conventions in format.py and branch.py.  This also makes explicit a bunch of import dependencies like bzrlib.inventory that were only implicitly imported before.

This is going to be really confusing. Loom has its own 'branch'
module, so please don't do that. (Not to mention that I am not
convinced by the bzr convention anyhow. If you feel you must do that,
please do _mod_bzrlib_branch or something to make in unambiguous.
Personally though, import bzrlib.branch is pretty nice.

>  * uses lazy_import where appropriate in format.py and branch.py (and don't use it when not helpful... register_formats is always called by __init__.py so bzrlib.branch will unavoidably be imported).

I would keep it lazy myself, if we fix bzrlib to not load
bzrlib.branch when registering formats, it would be a shame to have to
rework this because of an arbitrary change

-Rob