Merge lp:~allenap/maas/find-early-imports into lp:~maas-committers/maas/trunk

[Gavin Panella]

Check for imports that appear before __all__ and signal them as lint.

Strictly this isn't a problem for Python, it's a problem for MAAS's import formatter. Until we update it this simple script will help keep the "front matter" consistent.

Something I wrote over the weekend while nursing a cold and feeling sorry for myself.

[Blake Rouse (blake-rouse)]

Looks good. Surprised that so many files had that after imports.

review: Approve

[Mike Pontillo (mpontillo)]

+1; thanks for doing this. To be honest, most of these are probably my fault. PyCharm automatically imports things above __all__ and I constantly have to re-review my code and move them down so the imports formatter sorts them properly.

[Gavin Panella (allenap)]

> +1; thanks for doing this. To be honest, most of these are probably my
> fault. PyCharm automatically imports things above __all__ and I
> constantly have to re-review my code and move them down so the imports
> formatter sorts them properly.

I don't think PyCharm's doing anything wrong, it just doesn't gel with
our (wonky) tooling right now. I should have invested my time fixing
that rather than writing this, but I wasn't all with it this weekend.

=== modified file 'Makefile'
--- Makefile	2016-10-12 15:26:17 +0000
+++ Makefile	2016-10-17 06:51:39 +0000
@@ -285,8 +285,7 @@
 
 # Python lint checks are time-intensive, but flake8 now knows how to run
 # parallel jobs, and does so by default.
-lint-py: sources = \
-    setup.py $(wildcard contrib/*.py) src templates twisted utilities etc
+lint-py: sources = setup.py src templates twisted
 lint-py: bin/flake8
 	@find $(sources) -name '*.py' \
 	  ! -path '*/migrations/*' ! -path '*/south_migrations/*' -print0 \
@@ -296,8 +295,7 @@
 # be close to 10 but MAAS has many functions that are over that so we
 # start with a much higher number. Over time we can ratchet it down.
 lint-py-complexity: maximum=26
-lint-py-complexity: sources = \
-    setup.py $(wildcard contrib/*.py) src templates twisted utilities etc
+lint-py-complexity: sources = setup.py src templates twisted
 lint-py-complexity: bin/flake8
 	@find $(sources) -name '*.py' \
 	  ! -path '*/migrations/*' ! -path '*/south_migrations/*' \
@@ -306,8 +304,12 @@
 	              --isolated --max-complexity=$(maximum)
 
 # Statically check imports against policy.
+lint-py-imports: sources = setup.py src templates twisted
 lint-py-imports:
 	@utilities/check-imports
+	@find $(sources) -name '*.py' \
+	  ! -path '*/migrations/*' ! -path '*/south_migrations/*' \
+	  -print0 | xargs -r0 utilities/find-early-imports
 
 lint-doc:
 	@utilities/doc-lint

=== modified file 'src/maasserver/api/tests/test_ipaddresses.py'
--- src/maasserver/api/tests/test_ipaddresses.py	2016-10-12 17:50:40 +0000
+++ src/maasserver/api/tests/test_ipaddresses.py	2016-10-17 06:51:39 +0000
@@ -2,8 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for IP addresses API."""
-from testtools.matchers import HasLength
-
 
 __all__ = []
 
@@ -26,7 +24,10 @@
 from maasserver.utils.orm import reload_object
 from maastesting.matchers import DocTestMatches
 from netaddr import IPAddress
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    HasLength,
+)
 
 
 class TestIPAddressesAPI(APITestCase.ForUserAndAdmin):

=== modified file 'src/maasserver/forms_iprange.py'
--- src/maasserver/forms_iprange.py	2016-05-12 19:07:37 +0000
+++ src/maasserver/forms_iprange.py	2016-10-17 06:51:39 +0000
@@ -2,14 +2,13 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """IPRange form."""
-from maasserver.models import Subnet
-
 
 __all__ = [
     "IPRangeForm",
 ]
 
 from maasserver.forms import MAASModelForm
+from maasserver.models import Subnet
 from maasserver.models.iprange import IPRange
 
 

=== modified file 'src/maasserver/models/migrations/create_default_storage_layout.py'
--- src/maasserver/models/migrations/create_default_storage_layout.py	2016-06-22 17:03:02 +0000
+++ src/maasserver/models/migrations/create_default_storage_layout.py	2016-10-17 06:51:39 +0000
@@ -16,15 +16,6 @@
 do it, to ensure that the migrations meet validation requirements.)
 """
 
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-str = None
-
-__metaclass__ = type
 __all__ = [
     "clear_full_storage_configuration",
 ]

=== modified file 'src/maasserver/models/tests/test_template.py'
--- src/maasserver/models/tests/test_template.py	2016-03-25 13:52:27 +0000
+++ src/maasserver/models/tests/test_template.py	2016-10-17 06:51:39 +0000
@@ -2,15 +2,13 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the Template model."""
+
+__all__ = []
+
 from maasserver.models import VersionedTextFile
-from testtools.matchers import Equals
-
-
-__all__ = []
-
-
 from maasserver.models.template import Template
 from maasserver.testing.testcase import MAASServerTestCase
+from testtools.matchers import Equals
 
 
 class TemplateTest(MAASServerTestCase):

=== modified file 'src/maasserver/rpc/rackcontrollers.py'
--- src/maasserver/rpc/rackcontrollers.py	2016-10-12 15:26:17 +0000
+++ src/maasserver/rpc/rackcontrollers.py	2016-10-17 06:51:39 +0000
@@ -2,8 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """RPC helpers relating to rack controllers."""
-from provisioningserver.rpc.exceptions import NoSuchNode
-
 
 __all__ = [
     "handle_upgrade",
@@ -16,9 +14,9 @@
 
 from django.db.models import Q
 from maasserver import (
+    is_master_process,
     locks,
     worker_user,
-    is_master_process,
 )
 from maasserver.enum import NODE_TYPE
 from maasserver.models import (
@@ -36,6 +34,7 @@
     with_connection,
 )
 from provisioningserver.logger import get_maas_logger
+from provisioningserver.rpc.exceptions import NoSuchNode
 from provisioningserver.utils import typed
 from provisioningserver.utils.twisted import synchronous
 

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2016-09-02 16:20:15 +0000
+++ src/maasserver/urls_api.py	2016-10-17 06:51:39 +0000
@@ -2,11 +2,6 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """URL API routing configuration."""
-from maasserver.api.discoveries import (
-    DiscoveriesHandler,
-    DiscoveryHandler,
-)
-
 
 __all__ = []
 
@@ -53,6 +48,10 @@
     DHCPSnippetHandler,
     DHCPSnippetsHandler,
 )
+from maasserver.api.discoveries import (
+    DiscoveriesHandler,
+    DiscoveryHandler,
+)
 from maasserver.api.dnsresourcerecords import (
     DNSResourceRecordHandler,
     DNSResourceRecordsHandler,

=== modified file 'src/maasserver/utils/dns.py'
--- src/maasserver/utils/dns.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/utils/dns.py	2016-10-17 06:51:39 +0000
@@ -3,6 +3,10 @@
 
 """DNS-related utilities."""
 
+__all__ = [
+    'validate_hostname',
+    ]
+
 import re
 
 from django.core.exceptions import ValidationError
@@ -12,11 +16,6 @@
 )
 
 
-__all__ = [
-    'validate_hostname',
-    ]
-
-
 def validate_domain_name(name):
     """Validator for domain names.
 

=== modified file 'src/maasserver/utils/tests/test_dns.py'
--- src/maasserver/utils/tests/test_dns.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/utils/tests/test_dns.py	2016-10-17 06:51:39 +0000
@@ -3,6 +3,8 @@
 
 """Test DNS utilities."""
 
+__all__ = []
+
 from math import pow
 
 from django.core.exceptions import ValidationError
@@ -11,19 +13,14 @@
     validate_domain_name,
     validate_hostname,
 )
+from maastesting.factory import factory
+from maastesting.testcase import MAASTestCase
 from testtools.matchers import (
     Equals,
     HasLength,
 )
 
 
-__all__ = []
-
-
-from maastesting.factory import factory
-from maastesting.testcase import MAASTestCase
-
-
 class TestHostnameValidator(MAASTestCase):
     """Tests for the validation of hostnames.
 

=== modified file 'src/provisioningserver/templates/commissioning-user-data/snippets/maas_api_helper.py'
--- src/provisioningserver/templates/commissioning-user-data/snippets/maas_api_helper.py	2015-12-15 16:55:41 +0000
+++ src/provisioningserver/templates/commissioning-user-data/snippets/maas_api_helper.py	2016-10-17 06:51:39 +0000
@@ -1,3 +1,8 @@
+__all__ = [
+    'geturl',
+    'read_config',
+    ]
+
 from email.utils import parsedate
 import sys
 import time
@@ -9,12 +14,6 @@
 import yaml
 
 
-__all__ = [
-    'geturl',
-    'read_config',
-    ]
-
-
 def read_config(url, creds):
     """Read cloud-init config from given `url` into `creds` dict.
 

=== modified file 'src/provisioningserver/utils/isc.py'
--- src/provisioningserver/utils/isc.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/utils/isc.py	2016-10-17 06:51:39 +0000
@@ -28,9 +28,6 @@
 # ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 # POSSIBILITY OF SUCH DAMAGE.
 
-from collections import OrderedDict
-
-
 __all__ = [
     'ISCParseException',
     'make_isc_string',
@@ -38,6 +35,7 @@
     'read_isc_file',
 ]
 
+from collections import OrderedDict
 import copy
 
 

=== added file 'utilities/find-early-imports'
--- utilities/find-early-imports	1970-01-01 00:00:00 +0000
+++ utilities/find-early-imports	2016-10-17 06:51:39 +0000
@@ -0,0 +1,54 @@
+#!/usr/bin/env python3.5
+# -*- mode: python -*-
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Find imports that occur before `__all__`, if the latter is present.
+
+Exits non-zero if any are found.
+"""
+
+import argparse
+import sys
+import tokenize
+
+
+def find_early_imports(filepath):
+    has_early_import = has_all_declaration = False
+    with open(filepath, "rb") as fd:
+        for token in tokenize.tokenize(fd.readline):
+            if token.type == tokenize.NAME:
+                if token.string in {"import", "from"}:
+                    if not has_all_declaration:
+                        has_early_import = True
+                elif token.string == "__all__":
+                    has_all_declaration = True
+                    if not has_early_import:
+                        break  # Short-circuit.
+                else:
+                    pass  # Keep looking.
+    return has_all_declaration and has_early_import
+
+
+argument_parser = argparse.ArgumentParser(
+    formatter_class=argparse.RawDescriptionHelpFormatter,
+    description=__doc__)
+argument_parser.add_argument(
+    "filenames", nargs="+", metavar="FILENAME")
+
+
+if __name__ == '__main__':
+    options = argument_parser.parse_args()
+
+    has_early_imports = False
+    for filename in options.filenames:
+        if find_early_imports(filename):
+            if has_early_imports:
+                print(filename)
+            else:
+                has_early_imports = True
+                print("Early imports found:", file=sys.stderr)
+                print(filename)
+
+    raise SystemExit(
+        1 if has_early_imports else 0)