Code review comment for lp:~ddstreet/launchpadlib/nosudo

Looks mostly good, but tox reports that this fails tests on Python 2.7 and 3.5 like this:

Failure in test test_credentials_save_failed (launchpadlib.tests.test_launchpad.TestCredenitialSaveFailedCallback)
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/cjwatson/src/canonical/launchpadlib/launchpadlib/.tox/py27/local/lib/python2.7/site-packages/launchpadlib/tests/test_launchpad.py", line 632, in test_credentials_save_failed
    self.assertEqual(len(callback_called), 1)
  File "/usr/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 0 != 1

Failure in test test_default_credentials_save_failed_is_to_raise_exception (launchpadlib.tests.test_launchpad.TestCredenitialSaveFailedCallback)
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/cjwatson/src/canonical/launchpadlib/launchpadlib/.tox/py27/local/lib/python2.7/site-packages/launchpadlib/tests/test_launchpad.py", line 644, in test_default_credentials_save_failed_is_to_raise_exception
    launchpadlib_dir=launchpadlib_dir)
  File "/usr/lib/python2.7/unittest/case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "/usr/lib/python2.7/unittest/case.py", line 116, in __exit__
    "{0} not raised".format(exc_name))
AssertionError: RuntimeError not raised

This is because 2.7 and 3.5 are no longer supported by the version of keyring that has your patch to add the NoKeyringError exception, so tox picks an older version of keyring: in other words, this is indicating a slight incompatibility with the older keyring version.

Things seem to work well if I apply this patch on top:

=== modified file 'src/launchpadlib/credentials.py'
--- old/src/launchpadlib/credentials.py 2020-03-30 15:39:23 +0000
+++ new/src/launchpadlib/credentials.py 2020-04-08 15:38:53 +0000
@@ -393,7 +393,12 @@ class KeyringCredentialStore(CredentialS
         try:
             keyring.set_password(
                 'launchpadlib', unique_key, serialized.decode('utf-8'))
- except NoKeyringError:
+ except NoKeyringError as e:
+ # keyring < 21.2.0 raises RuntimeError rather than anything more
+ # specific. Make sure it's the exception we're interested in.
+ if (NoKeyringError == RuntimeError and
+ 'No recommended backend was available' not in str(e)):
+ raise
             if self._fallback:
                 self._fallback.save(credentials, unique_key)
             else:
@@ -405,7 +410,12 @@ class KeyringCredentialStore(CredentialS
         try:
             credential_string = keyring.get_password(
                 'launchpadlib', unique_key)
- except NoKeyringError:
+ except NoKeyringError as e:
+ # keyring < 21.2.0 raises RuntimeError rather than anything more
+ # specific. Make sure it's the exception we're interested in.
+ if (NoKeyringError == RuntimeError and
+ 'No recommended backend was available' not in str(e)):
+ raise
             if self._fallback:
                 return self._fallback.load(unique_key)
             else:

Does that look OK to you (I mean, it's fairly horrible, but it does the job)? If so could you apply it please?

(And yes, we do plan to move our various bzr branches to git, but we have a bit too much to do at the moment so haven't got round to it yet.)