Merge ~cjwatson/launchpad:deconfuse-isort into launchpad:master

[Colin Watson]

Adjust various imports to confuse isort a little less

Move non-circular imports to their usual place near the top of the file. Avoid `cimport` as a local variable because `isort` gets that confused with the Cython keyword. Add `# isort: split` in a few cases where we need to stop `isort` moving things around.

[Cristian Gonzalez (cristiangsp)]

Looks good!

review: Approve

diff --git a/lib/contrib/glock.py b/lib/contrib/glock.py
index de39d12..69def87 100644
--- a/lib/contrib/glock.py
+++ b/lib/contrib/glock.py
@@ -29,7 +29,6 @@ __doc__ += '\n@author: %s (U{%s})\n@version: %s' % (__author__[0],
                                             __author__[1], __version__)
 __all__ = ['GlobalLock', 'GlobalLockError', 'LockAlreadyAcquired', 'NotOwner']
 
-# Imports:
 import sys, string, os, errno, re
 
 # System-dependent imports for locking implementation:
diff --git a/lib/lp/app/browser/tests/test_vocabulary.py b/lib/lp/app/browser/tests/test_vocabulary.py
index 2904d8c..7265281 100644
--- a/lib/lp/app/browser/tests/test_vocabulary.py
+++ b/lib/lp/app/browser/tests/test_vocabulary.py
@@ -328,9 +328,7 @@ class TestProductPickerEntrySourceAdapter(TestCaseWithFactory):
     def test_provides_commercial_subscription_expired(self):
         product = self.factory.makeProduct(name='fnord')
         self.factory.makeCommercialSubscription(product)
-        import datetime
-        import pytz
-        then = datetime.datetime(2005, 6, 15, 0, 0, 0, 0, pytz.UTC)
+        then = datetime(2005, 6, 15, 0, 0, 0, 0, pytz.UTC)
         with celebrity_logged_in('admin'):
             product.commercial_subscription.date_expires = then
         self.assertEqual(
diff --git a/lib/lp/code/browser/tests/test_codeimport.py b/lib/lp/code/browser/tests/test_codeimport.py
index f8b1470..0dd76c6 100644
--- a/lib/lp/code/browser/tests/test_codeimport.py
+++ b/lib/lp/code/browser/tests/test_codeimport.py
@@ -94,48 +94,48 @@ class TestImportDetails(TestCaseWithFactory):
 
     def test_other_users_are_forbidden_to_change_codeimport(self):
         # Unauthorized users are forbidden to edit an import.
-        cimport = self.factory.makeCodeImport()
+        code_import = self.factory.makeCodeImport()
         another_person = self.factory.makePerson()
         with person_logged_in(another_person):
             self.assertRaises(
-                Unauthorized, create_initialized_view, cimport.branch,
+                Unauthorized, create_initialized_view, code_import.branch,
                 '+edit-import')
 
     def test_branch_owner_of_import_can_edit_it(self):
         # Owners are allowed to edit code import.
-        cimport = self.factory.makeCodeImport()
-        with person_logged_in(cimport.branch.owner):
+        code_import = self.factory.makeCodeImport()
+        with person_logged_in(code_import.branch.owner):
             view = create_initialized_view(
-                cimport.branch, '+edit-import', form={
+                code_import.branch, '+edit-import', form={
                     "field.actions.update": "update",
                     "field.url": "http://foo.test"
                 })
             self.assertEqual([], view.errors)
-            self.assertEqual('http://foo.test', cimport.url)
+            self.assertEqual('http://foo.test', code_import.url)
 
     def test_branch_owner_of_import_cannot_change_status(self):
         # Owners are allowed to edit code import.
-        cimport = self.factory.makeCodeImport()
-        original_url = cimport.url
-        with person_logged_in(cimport.branch.owner):
+        code_import = self.factory.makeCodeImport()
+        original_url = code_import.url
+        with person_logged_in(code_import.branch.owner):
             view = create_initialized_view(
-                cimport.branch, '+edit-import', form={
+                code_import.branch, '+edit-import', form={
                     "field.actions.suspend": "Suspend",
                     "field.url": "http://foo.test"
                 })
             self.assertEqual([], view.errors)
-            self.assertEqual(original_url, cimport.url)
+            self.assertEqual(original_url, code_import.url)
 
     def test_admin_can_change_code_import_status(self):
         # Owners are allowed to edit code import.
-        cimport = self.factory.makeCodeImport()
+        code_import = self.factory.makeCodeImport()
         with admin_logged_in():
             view = create_initialized_view(
-                cimport.branch, '+edit-import', form={
+                code_import.branch, '+edit-import', form={
                     "field.actions.suspend": "Suspend",
                     "field.url": "http://foo.test"
                 })
             self.assertEqual([], view.errors)
-            self.assertEqual("http://foo.test", cimport.url)
+            self.assertEqual("http://foo.test", code_import.url)
             self.assertEqual(
-                CodeImportReviewStatus.SUSPENDED, cimport.review_status)
+                CodeImportReviewStatus.SUSPENDED, code_import.review_status)
diff --git a/lib/lp/code/bzr.py b/lib/lp/code/bzr.py
index e823fc2..8e2e264 100644
--- a/lib/lp/code/bzr.py
+++ b/lib/lp/code/bzr.py
@@ -19,7 +19,7 @@ __all__ = [
 
 # FIRST Ensure correct plugins are loaded. Do not delete this comment or the
 # line below this comment.
-import lp.codehosting # noqa: F401
+import lp.codehosting  # noqa: F401  # isort: split
 
 from breezy.branch import UnstackableBranchFormat
 from breezy.bzr.branch import (
diff --git a/lib/lp/code/interfaces/branch.py b/lib/lp/code/interfaces/branch.py
index 45c8086..b8d52e5 100644
--- a/lib/lp/code/interfaces/branch.py
+++ b/lib/lp/code/interfaces/branch.py
@@ -50,6 +50,7 @@ from lazr.restful.fields import (
     ReferenceChoice,
     )
 from lazr.restful.interface import copy_field
+from lazr.uri import URI
 from six.moves import http_client
 from zope.component import getUtility
 from zope.interface import (
@@ -157,7 +158,6 @@ class BranchURIField(URIField):
     def _validate(self, value):
         # import here to avoid circular import
         from lp.services.webapp import canonical_url
-        from lazr.uri import URI
 
         # Can't use super-- this derives from an old-style class
         URIField._validate(self, value)
diff --git a/lib/lp/code/interfaces/webservice.py b/lib/lp/code/interfaces/webservice.py
index 428c998..02546e9 100644
--- a/lib/lp/code/interfaces/webservice.py
+++ b/lib/lp/code/interfaces/webservice.py
@@ -35,6 +35,9 @@ __all__ = [
     'ISourcePackageRecipeBuild',
     ]
 
+# XXX: JonathanLange 2010-11-09 bug=673083: Legacy work-around for circular
+# import bugs.  Break this up into a per-package thing.
+from lp import _schema_circular_imports  # noqa: F401
 # The exceptions are imported so that they can produce the special
 # status code defined by error_status when they are raised.
 from lp.code.errors import (
@@ -70,9 +73,3 @@ from lp.code.interfaces.sourcepackagerecipe import ISourcePackageRecipe
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuild,
     )
-
-
-# XXX: JonathanLange 2010-11-09 bug=673083: Legacy work-around for circular
-# import bugs.  Break this up into a per-package thing.
-from lp import _schema_circular_imports
-_schema_circular_imports
diff --git a/lib/lp/codehosting/puller/worker.py b/lib/lp/codehosting/puller/worker.py
index 4ccfcbe..e3c23a6 100644
--- a/lib/lp/codehosting/puller/worker.py
+++ b/lib/lp/codehosting/puller/worker.py
@@ -8,7 +8,7 @@ import sys
 
 # FIRST Ensure correct plugins are loaded. Do not delete this comment or the
 # line below this comment.
-import lp.codehosting  # noqa: F401
+import lp.codehosting  # noqa: F401  # isort: split
 
 from breezy import (
     errors,
diff --git a/lib/lp/registry/tests/test_prf_filter.py b/lib/lp/registry/tests/test_prf_filter.py
index 1dc0d24..093dc79 100644
--- a/lib/lp/registry/tests/test_prf_filter.py
+++ b/lib/lp/registry/tests/test_prf_filter.py
@@ -3,24 +3,24 @@
 
 """Tests for lp.registry.scripts.productreleasefinder.filter."""
 
+import logging
 import unittest
 
+from lp.registry.scripts.productreleasefinder.filter import (
+    Filter,
+    FilterPattern,
+    )
+
 
 class Filter_Logging(unittest.TestCase):
     def testCreatesDefaultLogger(self):
         """Filter creates a default logger."""
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter)
-        from logging import Logger
         f = Filter()
-        self.assertTrue(isinstance(f.log, Logger))
+        self.assertTrue(isinstance(f.log, logging.Logger))
 
     def testCreatesChildLogger(self):
         """Filter creates a child logger if given a parent."""
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter)
-        from logging import getLogger
-        parent = getLogger("foo")
+        parent = logging.getLogger("foo")
         f = Filter(log_parent=parent)
         self.assertEqual(f.log.parent, parent)
 
@@ -28,15 +28,11 @@ class Filter_Logging(unittest.TestCase):
 class Filter_Init(unittest.TestCase):
     def testDefaultFiltersProperty(self):
         """Filter constructor initializes filters property to empty dict."""
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter)
         f = Filter()
         self.assertEqual(f.filters, [])
 
     def testFiltersPropertyGiven(self):
         """Filter constructor accepts argument to set filters property."""
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter)
         f = Filter(["wibble"])
         self.assertEqual(len(f.filters), 1)
         self.assertEqual(f.filters[0], "wibble")
@@ -45,14 +41,10 @@ class Filter_Init(unittest.TestCase):
 class Filter_CheckUrl(unittest.TestCase):
     def testNoFilters(self):
         """Filter.check returns None if there are no filters."""
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter)
         f = Filter()
         self.assertEqual(f.check("file:///subdir/file"), None)
 
     def makeFilter(self, key, urlglob):
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter, FilterPattern)
         pattern = FilterPattern(key, urlglob)
         return Filter([pattern])
 
@@ -85,8 +77,6 @@ class Filter_CheckUrl(unittest.TestCase):
 class Filter_IsPossibleParentUrl(unittest.TestCase):
 
     def makeFilter(self, key, urlglob):
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter, FilterPattern)
         pattern = FilterPattern(key, urlglob)
         return Filter([pattern])
 
diff --git a/lib/lp/registry/tests/test_prf_hose.py b/lib/lp/registry/tests/test_prf_hose.py
index c679cc8..f56eb57 100644
--- a/lib/lp/registry/tests/test_prf_hose.py
+++ b/lib/lp/registry/tests/test_prf_hose.py
@@ -3,11 +3,17 @@
 
 """Tests for lp.registry.scripts.productreleasefinder.hose."""
 
+import logging
 import os
 import shutil
 import tempfile
 import unittest
 
+from lp.registry.scripts.productreleasefinder.filter import (
+    Filter,
+    FilterPattern,
+    )
+from lp.registry.scripts.productreleasefinder.hose import Hose
 from lp.testing import reset_logging
 
 
@@ -68,16 +74,12 @@ class InstrumentedMethodObserver:
 class Hose_Logging(unittest.TestCase):
     def testCreatesDefaultLogger(self):
         """Hose creates a default logger."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
-        from logging import Logger
         h = Hose()
-        self.assertTrue(isinstance(h.log, Logger))
+        self.assertTrue(isinstance(h.log, logging.Logger))
 
     def testCreatesChildLogger(self):
         """Hose creates a child logger if given a parent."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
-        from logging import getLogger
-        parent = getLogger("foo")
+        parent = logging.getLogger("foo")
         h = Hose(log_parent=parent)
         self.assertEqual(h.log.parent, parent)
 
@@ -85,23 +87,16 @@ class Hose_Logging(unittest.TestCase):
 class Hose_Filter(unittest.TestCase):
     def testCreatesFilterObject(self):
         """Hose creates a Filter object."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
-        from lp.registry.scripts.productreleasefinder.filter import (
-            Filter)
         h = Hose()
         self.assertTrue(isinstance(h.filter, Filter))
 
     def testDefaultsFiltersToEmptyDict(self):
         """Hose creates Filter object with empty dictionary."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
         h = Hose()
         self.assertEqual(h.filter.filters, [])
 
     def testCreatesFiltersWithGiven(self):
         """Hose creates Filter object with dictionary given."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
-        from lp.registry.scripts.productreleasefinder.filter import (
-            FilterPattern)
         pattern = FilterPattern("foo", "http:e*")
         h = Hose([pattern])
         self.assertEqual(len(h.filter.filters), 1)
@@ -111,7 +106,6 @@ class Hose_Filter(unittest.TestCase):
 class Hose_Urls(unittest.TestCase):
     def testCallsReduceWork(self):
         """Hose constructor calls reduceWork function."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
         h = Hose.__new__(Hose)
 
         class Observer(InstrumentedMethodObserver):
@@ -128,9 +122,6 @@ class Hose_Urls(unittest.TestCase):
 
     def testPassesUrlList(self):
         """Hose constructor passes url list to reduceWork."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
-        from lp.registry.scripts.productreleasefinder.filter import (
-            FilterPattern)
         pattern = FilterPattern("foo", "http://archive.ubuntu.com/e*")
         h = Hose.__new__(Hose)
 
@@ -148,8 +139,6 @@ class Hose_Urls(unittest.TestCase):
 
     def testSetsUrlProperty(self):
         """Hose constructor sets urls property to reduceWork return value."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
-
         class TestHose(Hose):
             def reduceWork(self, url_list):
                 return "wibble"
@@ -161,20 +150,17 @@ class Hose_Urls(unittest.TestCase):
 class Hose_ReduceWork(unittest.TestCase):
     def testEmptyList(self):
         """Hose.reduceWork returns empty list when given one."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
         h = Hose()
         self.assertEqual(h.reduceWork([]), [])
 
     def testReducedList(self):
         """Hose.reduceWork returns same list when nothing to do."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
         h = Hose()
         self.assertEqual(h.reduceWork(["http://localhost/", "file:///usr/"]),
                          ["http://localhost/", "file:///usr/"])
 
     def testReducesList(self):
         """Hose.reduceWork removes children elements from list."""
-        from lp.registry.scripts.productreleasefinder.hose import Hose
         h = Hose()
         self.assertEqual(h.reduceWork(["http://localhost/",
                                        "http://localhost/foo/bar/",
@@ -219,9 +205,6 @@ class Hose_LimitWalk(unittest.TestCase):
             fp.close()
 
         # Run the hose over the test data
-        from lp.registry.scripts.productreleasefinder.hose import Hose
-        from lp.registry.scripts.productreleasefinder.filter import (
-            FilterPattern)
         pattern = FilterPattern("key", self.release_url +
                                 "/foo/1.*/source/foo-1.*.tar.gz")
         hose = Hose([pattern])
diff --git a/lib/lp/registry/tests/test_prf_log.py b/lib/lp/registry/tests/test_prf_log.py
index d55f54f..2d663a0 100644
--- a/lib/lp/registry/tests/test_prf_log.py
+++ b/lib/lp/registry/tests/test_prf_log.py
@@ -7,38 +7,32 @@
 
 __author__ = "Scott James Remnant <[e-mail]>"
 
+import logging
 import unittest
 
+from lp.registry.scripts.productreleasefinder.log import get_logger
+
 
 class GetLogger(unittest.TestCase):
     def testLogger(self):
         """get_logger returns a Logger instance."""
-        from lp.registry.scripts.productreleasefinder.log import get_logger
-        from logging import Logger
-        self.assertTrue(isinstance(get_logger("test"), Logger))
+        self.assertTrue(isinstance(get_logger("test"), logging.Logger))
 
     def testNoParent(self):
         """get_logger works if no parent is given."""
-        from lp.registry.scripts.productreleasefinder.log import get_logger
         self.assertEqual(get_logger("test").name, "test")
 
     def testRootParent(self):
         """get_logger works if root logger is given."""
-        from lp.registry.scripts.productreleasefinder.log import get_logger
-        from logging import root
-        self.assertEqual(get_logger("test", root).name, "test")
+        self.assertEqual(get_logger("test", logging.root).name, "test")
 
     def testNormalParent(self):
         """get_logger works if non-root logger is given."""
-        from lp.registry.scripts.productreleasefinder.log import get_logger
-        from logging import getLogger
-        parent = getLogger("foo")
+        parent = logging.getLogger("foo")
         self.assertEqual(get_logger("test", parent).name, "foo.test")
 
     def testDeepParent(self):
         """get_logger works if deep-level logger is given."""
-        from lp.registry.scripts.productreleasefinder.log import get_logger
-        from logging import getLogger
-        getLogger("foo")
-        parent2 = getLogger("foo.bar")
+        logging.getLogger("foo")
+        parent2 = logging.getLogger("foo.bar")
         self.assertEqual(get_logger("test", parent2).name, "foo.bar.test")
diff --git a/lib/lp/services/librarian/client.py b/lib/lp/services/librarian/client.py
index 6787768..a26e39f 100644
--- a/lib/lp/services/librarian/client.py
+++ b/lib/lp/services/librarian/client.py
@@ -37,6 +37,7 @@ from six.moves.urllib.parse import (
     urlunparse,
     )
 from six.moves.urllib.request import urlopen
+from sqlobject import SQLObjectNotFound
 from storm.store import Store
 from zope.interface import implementer
 
@@ -385,7 +386,7 @@ class FileDownloadClient:
             inaccessible.
         """
         from lp.services.librarian.model import LibraryFileAlias
-        from sqlobject import SQLObjectNotFound
+
         try:
             lfa = LibraryFileAlias.get(aliasID)
         except SQLObjectNotFound:
diff --git a/lib/lp/services/mail/notificationrecipientset.py b/lib/lp/services/mail/notificationrecipientset.py
index 7297da2..6004399 100644
--- a/lib/lp/services/mail/notificationrecipientset.py
+++ b/lib/lp/services/mail/notificationrecipientset.py
@@ -14,7 +14,10 @@ from operator import attrgetter
 
 import six
 from zope.interface import implementer
-from zope.security.proxy import isinstance as zope_isinstance
+from zope.security.proxy import (
+    isinstance as zope_isinstance,
+    removeSecurityProxy,
+    )
 
 from lp.registry.interfaces.person import IPerson
 from lp.services.mail.interfaces import (
@@ -107,8 +110,8 @@ class NotificationRecipientSet:
 
     def add(self, persons, reason, header):
         """See `INotificationRecipientSet`."""
-        from zope.security.proxy import removeSecurityProxy
         from lp.registry.model.person import get_recipients
+
         if (IPerson.providedBy(persons) or
                 zope_isinstance(persons, StubPerson)):
             persons = [persons]
@@ -143,8 +146,8 @@ class NotificationRecipientSet:
 
     def remove(self, persons):
         """See `INotificationRecipientSet`."""
-        from zope.security.proxy import removeSecurityProxy
         from lp.registry.model.person import get_recipients
+
         if IPerson.providedBy(persons):
             persons = [persons]
         for person in persons:
diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
index 2d07f26..f6e931b 100644
--- a/lib/lp/testing/layers.py
+++ b/lib/lp/testing/layers.py
@@ -78,6 +78,7 @@ from six.moves.urllib.error import (
     )
 from six.moves.urllib.parse import urlparse
 from six.moves.urllib.request import urlopen
+from storm.uri import URI
 from talisker.context import Context
 import transaction
 from webob.request import environ_from_url as orig_environ_from_url
@@ -935,9 +936,7 @@ class LaunchpadLayer(LibrarianLayer, MemcachedLayer, RabbitMQLayer):
         in the testSetUp().
         """
         if LaunchpadLayer._raw_sessiondb_connection is None:
-            from storm.uri import URI
-            from lp.services.webapp.adapter import (
-                LaunchpadSessionDatabase)
+            from lp.services.webapp.adapter import LaunchpadSessionDatabase
             launchpad_session_database = LaunchpadSessionDatabase(
                 URI('launchpad-session:'))
             LaunchpadLayer._raw_sessiondb_connection = (
diff --git a/lib/lp/translations/scripts/translations_to_branch.py b/lib/lp/translations/scripts/translations_to_branch.py
index 0e57a88..f07abd5 100644
--- a/lib/lp/translations/scripts/translations_to_branch.py
+++ b/lib/lp/translations/scripts/translations_to_branch.py
@@ -15,7 +15,7 @@ import os.path
 
 # FIRST Ensure correct plugins are loaded. Do not delete this comment or the
 # line below this comment.
-import lp.codehosting  # noqa: F401
+import lp.codehosting  # noqa: F401  # isort: split
 
 from breezy.errors import NotBranchError
 from breezy.revision import NULL_REVISION
diff --git a/lib/sqlobject/__init__.py b/lib/sqlobject/__init__.py
index 3e5f9e4..5450477 100644
--- a/lib/sqlobject/__init__.py
+++ b/lib/sqlobject/__init__.py
@@ -7,13 +7,14 @@ __metaclass__ = type
 
 # SKIP this file when reformatting, due to the sys mangling.
 import datetime
+import sys
 
 import six
 from storm.expr import SQL
 from storm.sqlobject import *  # noqa: F401,F403
 
+
 # Provide the same interface from these other locations.
-import sys
 sys.modules['sqlobject.joins'] = sys.modules['sqlobject']
 sys.modules['sqlobject.sqlbuilder'] = sys.modules['sqlobject']
 del sys