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


...

>> For this specific part, I don't see how:
>>
>> from bzrlib.lazy_import import lazy_import
>> lazy_import(globals(), """
>> from bzrlib.plugins.launchpad import lp_registration
>> """)
>>
>> Is a "whole bunch of hard".
> 
> It doesn't work -- see http://paste.ubuntu.com/342421/
> 
> I tried a bunch of things based on suggestions from the #bzr channel.
> Then, on poolie's recommendation, I gave up and moved everything to
> scoped imports.
> 
> After I gave up, spiv made a suggestion:
> 
> <spiv> jml: if you do "from from bzrlib.plugins.launchpad import
> lp_registration as _lazy_lp_registration" in the __init__.py, does
> that fix it?
> 
> I haven't tried it.
> 

The problem is that you are assigning a lazily imported object to
another reference, which we can't trap, and we can't replace the new
object. As an example

foo.py:
lazy_import("bar")

bar.py:
frob = 10

quux.py:
from foo import bar

bar.frob # bad mojo

However, doing the above is bad python anyway. If you want "bar" in
"quux" then you should be importing it directly.

I'm guessing you may be importing classes rather than modules, which
also isn't a great thing to do, and lazy import makes it worse.
(isinstance(foo, lazy_class) doesn't always work.)


...

>> You seem to be upset with the review, and I'm sorry if I've put you on
>> the defensive. I feel like having "bzr lp-login" share its information
>> with "bzr lp-mirror" is a reasonable request.
>>
> 
> I'm not at all upset with the review, and am sorry for seeming so.
> 
> It's a reasonable request, but I think it's actually quite difficult
> to do well, and that it's not necessary for landing this patch. I'll
> happily file a bug about it once the patch lands.
> 

Sure.


> ...
>>> Which is terrible.
>>>
>>> When I first wrote this branch, that was the only choice. Since then, I added features to the Launchpad API to get branches based on their URLs, so this logic is no longer needed. I've rewritten this code to use that API, which makes it much simpler and removes the need for the _service attribute.
>> So since launchpadlib has been updated, do we need to add version
>> checks/etc so that things 'break' in a clean manner. (If I have
>> launchpadlib 1.really-old installed, would I get a reasonable error
>> rather than something like AttributeError?)
>>
> 
> We do. I made a note to do this, but forgot to actually do it. It's
> now done, with tests.
> 
> I haven't been able to figure out how to write a test that checks to
> see what happens if launchpadlib is not available at all.
> 

def test_lp_mirror_fails(self):
  if launchpadlib_feature.available():
    return  # raise TestNotApplicable
  self.run_bzr_error(['you must install launchpadlib'],
     		     'lp-mirror')

Not that I think you have to do it.

...

> I've added some smoke tests. I'm not satisfied with them, but I think
> they're enough for landing for now. Once this lands, I'll file a bug
> about having a good testing strategy for commands that depend on
> launchpadlib.
> 
> jml

You've satisfied my concerns. Now if you could just make lplib a sane
install :).

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

iEYEARECAAYFAkspuq8ACgkQJdeBCYSNAAP/hACeJL2Z/1fVU7nUJuKMbZkNB0zq
+nwAoNSnaQLhNsPjj8s42Eb5r2vLtDxm
=Am5B
-----END PGP SIGNATURE-----