[MERGE] add a command for managing the Launchpad user ID.

Martin Pool mbp at canonical.com
Fri Oct 12 01:35:29 BST 2007


Martin Pool has voted resubmit.
Status is now: Resubmit
Comment:

+
+class UnknownLaunchpadUsername(errors.BzrError):
+    _fmt = "The user name %(user)s is not registered on Launchpad."
+
+
+class NoRegisteredSSHKeys(errors.BzrError):
+    _fmt = "The user %(user)s has not registered any SSH keys with 
Launchpad."
+

There are no tests for the formatting of these exceptions.
(You might think that is too trivial to test but it is easy to
get the format string wrong.)


+def get_lp_login(config=None):
+    """Return the user's Launchpad Username"""
+    if config is None:
+        config = GlobalConfig()
+
+    return config.get_user_option('launchpad_username')

Further to Aaron's comment, I think you at least need a docstring
about what should be passed as the config.  s/Username/username/


+
+def check_lp_login(username, transport=None):
+    """Check whether the given Launchpad username is okay.
+
+    This will check for both existance and whether the user has
+    uploaded SSH keys.
+    """
+
+    if transport is None:
+        transport = get_transport(LAUNCHPAD_BASE)
+

I think you should explain what transport is meant to be for.  If it
is only for testing, it would be more consistent with other tests for
you to hook and then reset the variable -- or at least make call it
_transport.

Also there is already an environment variable(?) saying Launchpad's
address, maybe that should be used here too?

Is there a test/guarantee in Launchpad that ~user/+sshkeys will always 
be supported?

Does this give a reasonable error if the server is unreachable?


+      Set the Launchpad ID of the current user::
+
+          bzr launchpad-login $USERNAME

That's not such a good example because it won't work on Windows.

@@ -129,10 +173,11 @@
      from unittest import TestSuite, TestLoader
      import test_register
      import test_lp_indirect
+    import test_account

Since this was written we've decided to expose plugins as e.g. 
bzrlib.plugins.launchpad, and not to use relative imports.  So can you 
change them all to fully-qualified names?

This should be in NEWS.



+    takes_options = [
+        Option('no-check',
+               "Don't check that the user name is valid."),
+        ]
+
+    def run(self, name=None, no_check=None):
+        import account
+        check_account = not no_check

Isn't there an easier way to saay this is a boolean that defaults to on?


+            else:
+                self.outf.write('No Launchpad user ID configured.\n')
+        else:

Maybe it should return 1 for failure?

+        import account

Relative import again.

Aside from that it looks ok.


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3Ca7e835d40710111629n64f361b9w20fe230a0e5a1519%40mail.gmail.com%3E



More information about the bazaar mailing list