Patchwork [2,of,2] import-checker: reset context to verify convention in function scope

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 15, 2015, 1:31 p.m.
Message ID <469bc0309277eed1a2e1.1447594288@mimosa>
Download mbox | patch
Permalink /patch/11404/
State Accepted
Headers show

Comments

Yuya Nishihara - Nov. 15, 2015, 1:31 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1446367323 -32400
#      Sun Nov 01 17:42:03 2015 +0900
# Node ID 469bc0309277eed1a2e1b1e2dda7c0e77da6cb7d
# Parent  24d83ae70e628ed137f5af4e253ab4204597a247
import-checker: reset context to verify convention in function scope

I got the following error by rewriting hgweb/webcommands.py to use
absolute_import. It is false-positive because the import line appears in
"help" function:

  hgweb/webcommands.py:1297: higher-level import should come first: mercurial

This patch makes the import checker aware of the function scope and apply
rules recursively.
Pierre-Yves David - Nov. 16, 2015, 9:56 p.m.
On 11/15/2015 05:31 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1446367323 -32400
> #      Sun Nov 01 17:42:03 2015 +0900
> # Node ID 469bc0309277eed1a2e1b1e2dda7c0e77da6cb7d
> # Parent  24d83ae70e628ed137f5af4e253ab4204597a247
> import-checker: reset context to verify convention in function scope

These seems simple enough, I've them pushed to the clowncopter.

CCing Greg if he has a differente opinion.
Gregory Szorc - Nov. 17, 2015, 7:37 p.m.
> On Nov 16, 2015, at 13:56, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
>> On 11/15/2015 05:31 AM, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1446367323 -32400
>> #      Sun Nov 01 17:42:03 2015 +0900
>> # Node ID 469bc0309277eed1a2e1b1e2dda7c0e77da6cb7d
>> # Parent  24d83ae70e628ed137f5af4e253ab4204597a247
>> import-checker: reset context to verify convention in function scope
> 
> These seems simple enough, I've them pushed to the clowncopter.
> 
> CCing Greg if he has a differente opinion.

I don't. LGTM.

Patch

diff --git a/contrib/import-checker.py b/contrib/import-checker.py
--- a/contrib/import-checker.py
+++ b/contrib/import-checker.py
@@ -1,6 +1,7 @@ 
 #!/usr/bin/env python
 
 import ast
+import collections
 import os
 import sys
 
@@ -37,6 +38,17 @@  def usingabsolute(root):
 
     return False
 
+def walklocal(root):
+    """Recursively yield all descendant nodes but not in a different scope"""
+    todo = collections.deque(ast.iter_child_nodes(root))
+    yield root, False
+    while todo:
+        node = todo.popleft()
+        newscope = isinstance(node, ast.FunctionDef)
+        if not newscope:
+            todo.extend(ast.iter_child_nodes(node))
+        yield node, newscope
+
 def dotted_name_of_path(path, trimpure=False):
     """Given a relative path to a source file, return its dotted module name.
 
@@ -324,7 +336,7 @@  def verify_import_convention(module, sou
     else:
         return verify_stdlib_on_own_line(root)
 
-def verify_modern_convention(module, root):
+def verify_modern_convention(module, root, root_col_offset=0):
     """Verify a file conforms to the modern import convention rules.
 
     The rules of the modern convention are:
@@ -361,10 +373,15 @@  def verify_modern_convention(module, roo
     # Relative import levels encountered so far.
     seenlevels = set()
 
-    for node in ast.walk(root):
+    for node, newscope in walklocal(root):
         def msg(fmt, *args):
             return (fmt % args, node.lineno)
-        if isinstance(node, ast.Import):
+        if newscope:
+            # Check for local imports in function
+            for r in verify_modern_convention(module, node,
+                                              node.col_offset + 4):
+                yield r
+        elif isinstance(node, ast.Import):
             # Disallow "import foo, bar" and require separate imports
             # for each module.
             if len(node.names) > 1:
@@ -375,7 +392,7 @@  def verify_modern_convention(module, roo
             asname = node.names[0].asname
 
             # Ignore sorting rules on imports inside blocks.
-            if node.col_offset == 0:
+            if node.col_offset == root_col_offset:
                 if lastname and name < lastname:
                     yield msg('imports not lexically sorted: %s < %s',
                               name, lastname)
@@ -384,7 +401,7 @@  def verify_modern_convention(module, roo
 
             # stdlib imports should be before local imports.
             stdlib = name in stdlib_modules
-            if stdlib and seenlocal and node.col_offset == 0:
+            if stdlib and seenlocal and node.col_offset == root_col_offset:
                 yield msg('stdlib import follows local import: %s', name)
 
             if not stdlib:
@@ -423,7 +440,7 @@  def verify_modern_convention(module, roo
 
             # Direct symbol import is only allowed from certain modules and
             # must occur before non-symbol imports.
-            if node.module and node.col_offset == 0:
+            if node.module and node.col_offset == root_col_offset:
                 if fullname not in allowsymbolimports:
                     yield msg('direct symbol import from %s', fullname)
 
@@ -436,7 +453,8 @@  def verify_modern_convention(module, roo
                 seennonsymbolrelative = True
 
                 # Only allow 1 group per level.
-                if node.level in seenlevels and node.col_offset == 0:
+                if (node.level in seenlevels
+                    and node.col_offset == root_col_offset):
                     yield msg('multiple "from %s import" statements',
                               '.' * node.level)
 
diff --git a/tests/test-module-imports.t b/tests/test-module-imports.t
--- a/tests/test-module-imports.t
+++ b/tests/test-module-imports.t
@@ -74,6 +74,17 @@  Run additional tests for the import chec
   > from . import levelpriority  # should not cause cycle
   > EOF
 
+  $ cat > testpackage/subpackage/localimport.py << EOF
+  > from __future__ import absolute_import
+  > from . import foo
+  > def bar():
+  >     # should not cause "higher-level import should come first"
+  >     from .. import unsorted
+  >     # but other errors should be detected
+  >     from .. import more
+  >     import testpackage.subpackage.levelpriority
+  > EOF
+
   $ cat > testpackage/sortedentries.py << EOF
   > from __future__ import absolute_import
   > from . import (
@@ -105,6 +116,8 @@  Run additional tests for the import chec
   testpackage/sortedentries.py:2: imports from testpackage not lexically sorted: bar < foo
   testpackage/stdafterlocal.py:3: stdlib import follows local import: os
   testpackage/subpackage/levelpriority.py:3: higher-level import should come first: testpackage
+  testpackage/subpackage/localimport.py:7: multiple "from .. import" statements
+  testpackage/subpackage/localimport.py:8: import should be relative: testpackage.subpackage.levelpriority
   testpackage/symbolimport.py:2: direct symbol import from testpackage.unsorted
   testpackage/unsorted.py:3: imports not lexically sorted: os < sys
   [1]