> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Jonathan Lange wrote:
> > Jonathan Lange has proposed merging lp:~jml/bzr/lp-login-oauth-2 into
> lp:bzr.
> >
> >     Requested reviews:
> >     bzr-core (bzr-core)
> >
> >
> > Hello,
> >
> > This branch adds a command 'lp-mirror' that requests Launchpad mirror a
> branch now. It uses launchpadlib, and is most interesting for the fact that it
> integrates launchpadlib dependencies into Bazaar.
> >
> > jml
> >
> 
> 
> === modified file 'bzrlib/plugins/launchpad/__init__.py'
> - --- bzrlib/plugins/launchpad/__init__.py      2009-12-07 15:47:05 +0000
> +++ bzrlib/plugins/launchpad/__init__.py        2009-12-09 21:04:18 +0000
> @@ -111,7 +111,7 @@
>              link_bug=None,
>              dry_run=False):
>          from bzrlib.plugins.launchpad.lp_registration import (
> - -            LaunchpadService, BranchRegistrationRequest,
> BranchBugLinkRequest,
> +            BranchRegistrationRequest, BranchBugLinkRequest,
>              DryRunLaunchpadService)
>          if public_url is None:
>              try:
> 
> ^- This change made me wonder, but it looks like we are importing it at
> the module level. But if we are, then why aren't we importing the rest.
> 
> Also, it seems like it might be nicer to lazily import lp_registration.
> I wouldn't expect a huge improvement to bzr startup time, but maybe
> something?
> 
> Anyway, nothing you have to tweak, just a surprise on my part.
> 

I tried to change this to use lazy_import. It turns out to be a whole bunch of hard.

> 
> Most importantly, I can confirm that with this patch, and without
> launchpadlib:
> 
> 1) 'bzr lp-open' still works
> 2) 'bzr lp-mirror' fails with:
> $ bzr lp-mirror
> bzr: ERROR: launchpad-mirror requires launchpadlib
> 
> Which is a nice clean error.
> 

Thanks.

I also confirmed that 'bzr rocks' doesn't try to import launchpadlib, or indeed anything expensive from the launchpad plugin. 

> I can't confirm that anything works, because I'm not sure (yet) how to
> install everything for launchpadlib on win32.
> 

:(

> 
> Some basic comments by just looking at the code:
> 
> +CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache')
> 
> Doesn't seem quite right for Windows. This does generally have a value,
> but it isn't the place where you would put that sort of thing as a
> Windows user.
> 

I've tweaked this a bit so that it behaves like config_dir(). I've also changed the unix path to be ~/.cache/launchpadlib, which is slightly more standards compliant.

No tests, sadly.

> 
> It also seems surprising that 'lp-login' doesn't actually use the same
> credentials path or storage that 'lp_api.login()' ends up using.
> 

Well, surprise is an emotion and who am I to belittle or even predict the feelings of others.

lp-login actually has nothing to do with lp-mirror or with launchpadlib. lp-login is just a crappy thing to enable some of the other Launchpad interactions, particularly the directory service.

One day, I'd like lp-login to do things with launchpadlib, but I don't really know what the desired behaviour is. I think it's a good and safe thing to defer this thinking to a later patch.

> Which means that after running "bzr lp-login" doing "bzr lp-mirror" will
>  (unless I'm misreading something) not actually work.
> 

I think you are misreading something. I can run lp-login and then lp-mirror.

> 
> def login(service, timeout=None, proxy_info=None):
>     """Log in to the Launchpad API.
> 
>     :return: The root `Launchpad` object from launchpadlib.
>     """
>     credential_path = _get_credential_path(service)
>     launchpad = _login_from_cache(
>         'bzr', _get_api_url(service), CACHE_DIRECTORY, credential_path,
>         timeout, proxy_info)
>     launchpad._service = service
> 
> ^- Something seems a bit odd in this line, too. Why are you setting a
> private member of a class? Given that _login_from_cache takes the
> service root, it would seem that it should already have a reasonable
> service.
> 

I don't actually need this line any more. See later for an explanation.

> I guess looking back at cmd_lp_mirror you have:
>         service = LaunchpadService()
>         lp_api = self._get_lp_api()
>         launchpad = lp_api.login(service)
> 
> 
> Which means that the LaunchpadService instance that login is using
> *isn't* the launchpadlib api one, but it is the one we have already
> coded in bzrlib.
> 

Yeah, that's the expected behaviour. There is no launchpadlib 'LaunchpadService' instance. There's only the LaunchpadService that's used by the rest of the Bazaar launchpad plugin.

I'm using that because it comes quite close to being an abstraction for "an instance of Launchpad".

> This seems like a significant point of failure, if the rest of
> launchpadlib commands expect whatever launchpadlib service, and then we
> are passing it whatever we have in bzrlib. (Certainly an opportunity for
> api skew...)
> 

login() expects something to identify which Launchpad instance to log in to. The rest of the launchpadlib functions expect a logged-in `Launchpad` object from launchpadlib.

> Though I guess stuff like "def load_branch()" is then grabbing access to
> that secret member:

Right. Except, not any more...

> +def load_branch(launchpad, branch):
> +    """Return the launchpadlib Branch object corresponding to 'branch'.
> +
> +    :param launchpad: The root `Launchpad` object from launchpadlib.
> +    :param branch: A `bzrlib.branch.Branch`.
> +    :raise NotLaunchpadBranch: If we cannot determine the Launchpad URL of
> +        `branch`.
> +    :return: A launchpadlib Branch object.
> +    """
> +    service = launchpad._service
> +    for url in branch.get_public_branch(), branch.get_push_location():
> +        if url is None:
> +            continue
> +        try:
> +            path = service._guess_branch_path(url)
> 
> ^- And then calling a private function on it, even...
> 

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.

> 
> There also doesn't seem to be any tests that 'bzr lp-mirror' exists or
> even does anything. (Which has a small advantage that you don't have to
> worry about skipping those tests when launchpadlib isn't available.)
> 

I'm clinging to that small advantage, since you haven't actually made a recommendation here.

jml