Patchwork import-checker: report local with stdlib late warning

login
register
mail settings
Submitter timeless@mozdev.org
Date March 2, 2016, 3:48 p.m.
Message ID <74f6a6f4a9c822220440.1456933696@waste.org>
Download mbox | patch
Permalink /patch/13538/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

timeless@mozdev.org - March 2, 2016, 3:48 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1456933134 0
#      Wed Mar 02 15:38:54 2016 +0000
# Node ID 74f6a6f4a9c82222044059cce91d8bda283e9989
# Parent  c7f89ad87baef87f00c507545dfd4cc824bc3131
import-checker: report local with stdlib late warning

Without this, developers have to figure it out on their own
Gregory Szorc - March 4, 2016, 6:34 a.m.
> On Mar 2, 2016, at 07:48, timeless <timeless@mozdev.org> wrote:
> 
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1456933134 0
> #      Wed Mar 02 15:38:54 2016 +0000
> # Node ID 74f6a6f4a9c82222044059cce91d8bda283e9989
> # Parent  c7f89ad87baef87f00c507545dfd4cc824bc3131
> import-checker: report local with stdlib late warning
> 
> Without this, developers have to figure it out on their own
> 
> diff --git a/contrib/import-checker.py b/contrib/import-checker.py
> --- a/contrib/import-checker.py
> +++ b/contrib/import-checker.py
> @@ -366,7 +366,7 @@
>     fromlocal = fromlocalfunc(module, localmods)
> 
>     # Whether a local/non-stdlib import has been performed.
> -    seenlocal = False
> +    seenlocal = None
>     # Whether a relative, non-symbol import has been seen.
>     seennonsymbolrelative = False
>     # The last name to be imported (for sorting).
> @@ -402,11 +402,14 @@
> 
>             # stdlib imports should be before local imports.
>             stdlib = name in stdlib_modules
> -            if stdlib and seenlocal and node.col_offset == root_col_offset:
> -                yield msg('stdlib import follows local import: %s', name)
> +            if (stdlib and seenlocal is not None and

I don't think you need the type compare on seenlocal here. Otherwise LGTM.

> +                node.col_offset == root_col_offset):
> +                yield msg('stdlib import "%s" follows local import: %s',
> +                          name,
> +                          seenlocal)
> 
>             if not stdlib:
> -                seenlocal = True
> +                seenlocal = name
> 
>             # Import of sibling modules should use relative imports.
>             topname = name.split('.')[0]
> @@ -437,7 +440,7 @@
>                 if not fullname or fullname in stdlib_modules:
>                     yield msg('relative import of stdlib module')
>                 else:
> -                    seenlocal = True
> +                    seenlocal = fullname
> 
>             # Direct symbol import is only allowed from certain modules and
>             # must occur before non-symbol imports.
> diff --git a/tests/test-check-module-imports.t b/tests/test-check-module-imports.t
> --- a/tests/test-check-module-imports.t
> +++ b/tests/test-check-module-imports.t
> @@ -125,7 +125,7 @@
>   testpackage/relativestdlib.py:2: relative import of stdlib module
>   testpackage/requirerelative.py:2: import should be relative: testpackage.unsorted
>   testpackage/sortedentries.py:2: imports from testpackage not lexically sorted: bar < foo
> -  testpackage/stdafterlocal.py:3: stdlib import follows local import: os
> +  testpackage/stdafterlocal.py:3: stdlib import "os" follows local import: testpackage
>   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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - March 4, 2016, 2:04 p.m.
On Thu, 3 Mar 2016 22:34:51 -0800, Gregory Szorc wrote:
> > On Mar 2, 2016, at 07:48, timeless <timeless@mozdev.org> wrote:
> > 
> > # HG changeset patch
> > # User timeless <timeless@mozdev.org>
> > # Date 1456933134 0
> > #      Wed Mar 02 15:38:54 2016 +0000
> > # Node ID 74f6a6f4a9c82222044059cce91d8bda283e9989
> > # Parent  c7f89ad87baef87f00c507545dfd4cc824bc3131
> > import-checker: report local with stdlib late warning
> > 
> > Without this, developers have to figure it out on their own
> > 
> > diff --git a/contrib/import-checker.py b/contrib/import-checker.py
> > --- a/contrib/import-checker.py
> > +++ b/contrib/import-checker.py
> > @@ -366,7 +366,7 @@
> >     fromlocal = fromlocalfunc(module, localmods)
> > 
> >     # Whether a local/non-stdlib import has been performed.
> > -    seenlocal = False
> > +    seenlocal = None
> >     # Whether a relative, non-symbol import has been seen.
> >     seennonsymbolrelative = False
> >     # The last name to be imported (for sorting).
> > @@ -402,11 +402,14 @@
> > 
> >             # stdlib imports should be before local imports.
> >             stdlib = name in stdlib_modules
> > -            if stdlib and seenlocal and node.col_offset == root_col_offset:
> > -                yield msg('stdlib import follows local import: %s', name)
> > +            if (stdlib and seenlocal is not None and
> 
> I don't think you need the type compare on seenlocal here. Otherwise LGTM.

Dropped the strict type comparison and pushed to the clowncopter, thanks!

Patch

diff --git a/contrib/import-checker.py b/contrib/import-checker.py
--- a/contrib/import-checker.py
+++ b/contrib/import-checker.py
@@ -366,7 +366,7 @@ 
     fromlocal = fromlocalfunc(module, localmods)
 
     # Whether a local/non-stdlib import has been performed.
-    seenlocal = False
+    seenlocal = None
     # Whether a relative, non-symbol import has been seen.
     seennonsymbolrelative = False
     # The last name to be imported (for sorting).
@@ -402,11 +402,14 @@ 
 
             # stdlib imports should be before local imports.
             stdlib = name in stdlib_modules
-            if stdlib and seenlocal and node.col_offset == root_col_offset:
-                yield msg('stdlib import follows local import: %s', name)
+            if (stdlib and seenlocal is not None and
+                node.col_offset == root_col_offset):
+                yield msg('stdlib import "%s" follows local import: %s',
+                          name,
+                          seenlocal)
 
             if not stdlib:
-                seenlocal = True
+                seenlocal = name
 
             # Import of sibling modules should use relative imports.
             topname = name.split('.')[0]
@@ -437,7 +440,7 @@ 
                 if not fullname or fullname in stdlib_modules:
                     yield msg('relative import of stdlib module')
                 else:
-                    seenlocal = True
+                    seenlocal = fullname
 
             # Direct symbol import is only allowed from certain modules and
             # must occur before non-symbol imports.
diff --git a/tests/test-check-module-imports.t b/tests/test-check-module-imports.t
--- a/tests/test-check-module-imports.t
+++ b/tests/test-check-module-imports.t
@@ -125,7 +125,7 @@ 
   testpackage/relativestdlib.py:2: relative import of stdlib module
   testpackage/requirerelative.py:2: import should be relative: testpackage.unsorted
   testpackage/sortedentries.py:2: imports from testpackage not lexically sorted: bar < foo
-  testpackage/stdafterlocal.py:3: stdlib import follows local import: os
+  testpackage/stdafterlocal.py:3: stdlib import "os" follows local import: testpackage
   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