Bazaar

Code review comment for lp:~jml/bzr/lp-login-oauth-2

lp-login-oauth-2
Merge into bzr.dev

Revision history for this message

John A Meinel (jameinel) wrote on 2009-12-09:

-----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.

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.

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.

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

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

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 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.

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...)

Though I guess stuff like "def load_branch()" is then grabbing access to
that secret member:
+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...

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.)

But the code structuring concerns me a bit.

review: needs_fixing

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

iEYEARECAAYFAksgHgwACgkQJdeBCYSNAAPSOQCgxgz2g/v5Osclx3WZ9l9tAxLR
uR8Ani8zvSGQ6fvBF+2vUn7YrHKKI9Hp
=VZ3v
-----END PGP SIGNATURE-----

-----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.

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.

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.

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

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

def login(service, timeout=None, proxy_info=None):
    """Log in to the Launchpad API.

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.

Though I guess stuff like "def load_branch()" is then grabbing access to
that secret member:
+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...

But the code structuring concerns me a bit.

review: needs_fixing

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

iEYEARECAAYFAksgHgwACgkQJdeBCYSNAAPSOQCgxgz2g/v5Osclx3WZ9l9tAxLR
uR8Ani8zvSGQ6fvBF+2vUn7YrHKKI9Hp
=VZ3v
-----END PGP SIGNATURE-----

review: Needs Fixing

« Back to merge proposal