wrong-import-position, wrong-import-order: Skip try-import cases and nested imports (#906)

27 files changed, 146 insertions, 19 deletions

diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index 1236daf65..98697372d 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -87,6 +87,7 @@ Order doesn't matter (not that much, at least ;)
 * Moisés López (Vauxoo): Support for deprecated-modules in modules not installed,
   Refactory wrong-import-order to integrate it with `isort` library
   Add check too-complex with mccabe for cyclomatic complexity
+  Refactory wrong-import-position to skip try-import and nested cases.
 
 * Luis Escobar (Vauxoo), Moisés López (Vauxoo): Add bad-docstring-quotes and docstring-first-line-empty
 
diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py
index 94a94a228..db672c13d 100644
--- a/pylint/checkers/imports.py
+++ b/pylint/checkers/imports.py
@@ -359,8 +359,10 @@ given file (report RP0402 must not be disabled)'}
         for name in names:
             self._check_deprecated_module(node, name)
             importedmodnode = self._get_imported_module(node, name)
-            if isinstance(node.scope(), astroid.Module):
+            if isinstance(node.parent, astroid.Module):
+                # Allow imports nested
                 self._check_position(node)
+            if isinstance(node.scope(), astroid.Module):
                 self._record_import(node, importedmodnode)
 
             if importedmodnode is None:
@@ -381,8 +383,10 @@ given file (report RP0402 must not be disabled)'}
 
         modnode = node.root()
         importedmodnode = self._get_imported_module(node, basename)
-        if isinstance(node.scope(), astroid.Module):
+        if isinstance(node.parent, astroid.Module):
+            # Allow imports nested
             self._check_position(node)
+        if isinstance(node.scope(), astroid.Module):
             self._record_import(node, importedmodnode)
         if importedmodnode is None:
             return
@@ -421,12 +425,16 @@ given file (report RP0402 must not be disabled)'}
             return
         if not isinstance(node.parent, astroid.Module):
             return
-        if any(node.nodes_of_class((astroid.Import, astroid.ImportFrom))):
+        nested_allowed = [astroid.TryExcept, astroid.TryFinally]
+        is_nested_allowed = [
+            allowed for allowed in nested_allowed if isinstance(node, allowed)]
+        if is_nested_allowed and \
+                any(node.nodes_of_class((astroid.Import, astroid.ImportFrom))):
             return
         self._first_non_import_node = node
 
     visit_tryfinally = visit_tryexcept = visit_assignattr = visit_assign \
-            = visit_ifexp = visit_comprehension = visit_if
+            = visit_ifexp = visit_comprehension = visit_expr = visit_if
 
     def visit_functiondef(self, node):
         # If it is the first non import instruction of the module, record it.
@@ -515,6 +523,8 @@ given file (report RP0402 must not be disabled)'}
         extern_imports = []
         local_imports = []
         std_imports = []
+        extern_not_nested = []
+        local_not_nested = []
         isort_obj = isort.SortImports(
             file_contents='', known_third_party=self.config.known_third_party,
             known_standard_library=self.config.known_standard_library,
@@ -524,26 +534,30 @@ given file (report RP0402 must not be disabled)'}
                 package = '.' + modname.split('.')[1]
             else:
                 package = modname.split('.')[0]
-
+            nested = not isinstance(node.parent, astroid.Module)
             import_category = isort_obj.place_module(package)
             if import_category in ('FUTURE', 'STDLIB'):
                 std_imports.append((node, package))
-                wrong_import = extern_imports or local_imports
+                wrong_import = extern_not_nested or local_not_nested
                 if self._is_fallback_import(node, wrong_import):
                     continue
-                if wrong_import:
+                if wrong_import and not nested:
                     self.add_message('wrong-import-order', node=node,
                                      args=('standard import "%s"' % node.as_string(),
                                            '"%s"' % wrong_import[0][0].as_string()))
             elif import_category in ('FIRSTPARTY', 'THIRDPARTY'):
                 extern_imports.append((node, package))
-                wrong_import = local_imports
-                if wrong_import:
+                if not nested:
+                    extern_not_nested.append((node, package))
+                wrong_import = local_not_nested
+                if wrong_import and not nested:
                     self.add_message('wrong-import-order', node=node,
                                      args=('external import "%s"' % node.as_string(),
                                            '"%s"' % wrong_import[0][0].as_string()))
             elif import_category == 'LOCALFOLDER':
                 local_imports.append((node, package))
+                if not nested:
+                    local_not_nested.append((node, package))
         return std_imports, extern_imports, local_imports
 
     def _get_imported_module(self, importnode, modname):
diff --git a/pylint/test/functional/import_error.py b/pylint/test/functional/import_error.py
index 8f7bb3f30..add0bf30e 100644
--- a/pylint/test/functional/import_error.py
+++ b/pylint/test/functional/import_error.py
@@ -1,5 +1,5 @@
 """ Test that import errors are detected. """

-# pylint: disable=invalid-name, unused-import, no-absolute-import, bare-except, broad-except, wrong-import-order

+# pylint: disable=invalid-name, unused-import, no-absolute-import, bare-except, broad-except, wrong-import-order, wrong-import-position

 import totally_missing # [import-error]

 

 try:

diff --git a/pylint/test/functional/logging_format_interpolation.py b/pylint/test/functional/logging_format_interpolation.py
index 5432d337c..0f8f64db3 100644
--- a/pylint/test/functional/logging_format_interpolation.py
+++ b/pylint/test/functional/logging_format_interpolation.py
@@ -1,4 +1,4 @@
-# pylint: disable=E1101, no-absolute-import, import-error,line-too-long, missing-docstring,wrong-import-order
+# pylint: disable=E1101, no-absolute-import, import-error,line-too-long, missing-docstring,wrong-import-order,wrong-import-position
 
 try:
     import __builtin__ as builtins
diff --git a/pylint/test/functional/no_name_in_module.py b/pylint/test/functional/no_name_in_module.py
index 44bb04d37..f64341a79 100644
--- a/pylint/test/functional/no_name_in_module.py
+++ b/pylint/test/functional/no_name_in_module.py
@@ -1,4 +1,4 @@
-#pylint: disable=W0401,W0611,no-absolute-import,invalid-name,import-error,bare-except,broad-except,wrong-import-order,ungrouped-imports
+#pylint: disable=W0401,W0611,no-absolute-import,invalid-name,import-error,bare-except,broad-except,wrong-import-order,ungrouped-imports,wrong-import-position
 """check unexistant names imported are reported"""
 from __future__ import print_function
 
diff --git a/pylint/test/functional/ungrouped_imports.py b/pylint/test/functional/ungrouped_imports.py
index 2204c8031..e631e60b1 100644
--- a/pylint/test/functional/ungrouped_imports.py
+++ b/pylint/test/functional/ungrouped_imports.py
@@ -1,5 +1,5 @@
 """Checks import order rule"""
-# pylint: disable=unused-import,relative-import,wrong-import-order,using-constant-test
+# pylint: disable=unused-import,relative-import,wrong-import-position,wrong-import-order,using-constant-test
 # pylint: disable=import-error
 import six
 import logging.config
diff --git a/pylint/test/functional/wrong_import_order.py b/pylint/test/functional/wrong_import_order.py
index 9b5686102..593f65420 100644
--- a/pylint/test/functional/wrong_import_order.py
+++ b/pylint/test/functional/wrong_import_order.py
@@ -5,6 +5,8 @@ try:
 except ImportError:
     import configparser
 
+import logging
+
 import six
 import os.path  # [wrong-import-order]
 from astroid import are_exclusive
@@ -18,3 +20,22 @@ import astroid  # [wrong-import-order]
 from . import package2
 from .package2 import Class2
 from ..package3 import Class3
+
+
+LOGGER = logging.getLogger(__name__)
+
+
+if LOGGER:
+    # imports nested skipped
+    from . import package4
+    import pprint
+    from pprint import PrettyPrinter
+
+
+try:
+    # imports nested skipped
+    from . import package4
+    import random
+    from random import division
+except ImportError:
+    LOGGER.info('A useful message here')
diff --git a/pylint/test/functional/wrong_import_order.txt b/pylint/test/functional/wrong_import_order.txt
index 6c6588805..a9a3e8985 100644
--- a/pylint/test/functional/wrong_import_order.txt
+++ b/pylint/test/functional/wrong_import_order.txt
@@ -1,5 +1,5 @@
-wrong-import-order:9::standard import "import os.path" comes before "from six.moves import configparser"
-wrong-import-order:11::standard import "import sys" comes before "from six.moves import configparser"
-wrong-import-order:12::standard import "import datetime" comes before "from six.moves import configparser"
-wrong-import-order:15::external import "import totally_missing" comes before "from .package import Class"
-wrong-import-order:17::external import "import astroid" comes before "from .package import Class"
+wrong-import-order:11::standard import "import os.path" comes before "import six"
+wrong-import-order:13::standard import "import sys" comes before "import six"
+wrong-import-order:14::standard import "import datetime" comes before "import six"
+wrong-import-order:17::external import "import totally_missing" comes before "from .package import Class"
+wrong-import-order:19::external import "import astroid" comes before "from .package import Class"
diff --git a/pylint/test/functional/wrong_import_position.py b/pylint/test/functional/wrong_import_position.py
index 269d7573c..0f8a92e84 100644
--- a/pylint/test/functional/wrong_import_position.py
+++ b/pylint/test/functional/wrong_import_position.py
@@ -19,7 +19,7 @@ else:
         def some_func(self):
             pass
 
-import six
+import six  # [wrong-import-position]
 
 CONSTANT = True
 
diff --git a/pylint/test/functional/wrong_import_position.txt b/pylint/test/functional/wrong_import_position.txt
index 5cde17dbb..e734ac5e5 100644
--- a/pylint/test/functional/wrong_import_position.txt
+++ b/pylint/test/functional/wrong_import_position.txt
@@ -1,3 +1,4 @@
+wrong-import-position:22::Import "import six" should be placed at the top of the module
 wrong-import-position:26::Import "import datetime" should be placed at the top of the module
 wrong-import-position:32::Import "import scipy" should be placed at the top of the module
 wrong-import-position:33::Import "import astroid" should be placed at the top of the module
diff --git a/pylint/test/functional/wrong_import_position10.py b/pylint/test/functional/wrong_import_position10.py
new file mode 100644
index 000000000..1dbbf4f9d
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position10.py
@@ -0,0 +1,17 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,wrong-import-order,using-constant-test
+# pylint: disable=import-error, too-few-public-methods,missing-docstring
+
+import os
+
+try:
+    import ast
+except ImportError:
+    def method(items):
+        """docstring"""
+        value = 0
+        for item in items:
+            value += item
+        return value
+
+import sys
diff --git a/pylint/test/functional/wrong_import_position11.py b/pylint/test/functional/wrong_import_position11.py
new file mode 100644
index 000000000..4a78d778c
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position11.py
@@ -0,0 +1,4 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+A = 1
+import os  # [wrong-import-position]
diff --git a/pylint/test/functional/wrong_import_position11.txt b/pylint/test/functional/wrong_import_position11.txt
new file mode 100644
index 000000000..01bb58421
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position11.txt
@@ -0,0 +1 @@
+wrong-import-position:4::Import "import os" should be placed at the top of the module
diff --git a/pylint/test/functional/wrong_import_position12.py b/pylint/test/functional/wrong_import_position12.py
new file mode 100644
index 000000000..cbaa083ce
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position12.py
@@ -0,0 +1,5 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level,pointless-string-statement
+"Two string"
+
+import os  # [wrong-import-position]
diff --git a/pylint/test/functional/wrong_import_position12.txt b/pylint/test/functional/wrong_import_position12.txt
new file mode 100644
index 000000000..6d560822b
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position12.txt
@@ -0,0 +1 @@
+wrong-import-position:5::Import "import os" should be placed at the top of the module
diff --git a/pylint/test/functional/wrong_import_position13.py b/pylint/test/functional/wrong_import_position13.py
new file mode 100644
index 000000000..11f40c60f
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position13.py
@@ -0,0 +1,4 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+A = 1
+from sys import x  # [wrong-import-position]
diff --git a/pylint/test/functional/wrong_import_position13.txt b/pylint/test/functional/wrong_import_position13.txt
new file mode 100644
index 000000000..f3def1900
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position13.txt
@@ -0,0 +1 @@
+wrong-import-position:4::Import "from sys import x" should be placed at the top of the module
diff --git a/pylint/test/functional/wrong_import_position14.py b/pylint/test/functional/wrong_import_position14.py
new file mode 100644
index 000000000..88ba5c646
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position14.py
@@ -0,0 +1,5 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level,undefined-variable
+if x:
+    import os
+import y  # [wrong-import-position]
diff --git a/pylint/test/functional/wrong_import_position14.txt b/pylint/test/functional/wrong_import_position14.txt
new file mode 100644
index 000000000..9fcdac503
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position14.txt
@@ -0,0 +1 @@
+wrong-import-position:5::Import "import y" should be placed at the top of the module
diff --git a/pylint/test/functional/wrong_import_position2.py b/pylint/test/functional/wrong_import_position2.py
new file mode 100644
index 000000000..65ad9e656
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position2.py
@@ -0,0 +1,10 @@
+"""Checks import order rule with nested non_import sentence"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+try:
+    from sys import argv
+except ImportError:
+    pass
+else:
+    pass
+
+import os
diff --git a/pylint/test/functional/wrong_import_position3.py b/pylint/test/functional/wrong_import_position3.py
new file mode 100644
index 000000000..d2cf68529
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position3.py
@@ -0,0 +1,3 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+import os
diff --git a/pylint/test/functional/wrong_import_position4.py b/pylint/test/functional/wrong_import_position4.py
new file mode 100644
index 000000000..3f1174f9d
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position4.py
@@ -0,0 +1,5 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level,unused-variable
+def method1():
+    """Method 1"""
+    import x
diff --git a/pylint/test/functional/wrong_import_position5.py b/pylint/test/functional/wrong_import_position5.py
new file mode 100644
index 000000000..b96a8a0e4
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position5.py
@@ -0,0 +1,4 @@
+r"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+
+import os
diff --git a/pylint/test/functional/wrong_import_position6.py b/pylint/test/functional/wrong_import_position6.py
new file mode 100644
index 000000000..694c109d1
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position6.py
@@ -0,0 +1,7 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level,undefined-variable
+
+import y
+
+if x:
+    import os
diff --git a/pylint/test/functional/wrong_import_position7.py b/pylint/test/functional/wrong_import_position7.py
new file mode 100644
index 000000000..bb5095603
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position7.py
@@ -0,0 +1,9 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+try:
+    import x
+except ImportError:
+    pass
+finally:
+    pass
+import y
diff --git a/pylint/test/functional/wrong_import_position8.py b/pylint/test/functional/wrong_import_position8.py
new file mode 100644
index 000000000..cd1028a1b
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position8.py
@@ -0,0 +1,4 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level,undefined-variable
+if x:
+    import os
diff --git a/pylint/test/functional/wrong_import_position9.py b/pylint/test/functional/wrong_import_position9.py
new file mode 100644
index 000000000..a08880b8a
--- /dev/null
+++ b/pylint/test/functional/wrong_import_position9.py
@@ -0,0 +1,9 @@
+"""Checks import position rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+import y
+try:
+    import x
+except ImportError:
+    pass
+else:
+    pass