Check imports are ordered (standard, 3rd party, local) and grouped by package

related to issue #692

14 files changed, 132 insertions, 10 deletions

diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index f05e053..276a63d 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -75,4 +75,4 @@ Order doesn't matter (not that much, at least ;)
 
 * Laura Medioni (Logilab, on behalf of the CNES): misplaced-comparison-constant,
   no-classmethod-decorator, no-staticmethod-decorator, too-many-nested-blocks,
-  too-many-boolean-expressions, unneeded-not
\ No newline at end of file
+  too-many-boolean-expressions, unneeded-not, wrong-import-order, ungrouped-imports
\ No newline at end of file
diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py
index 59c6ed0..fc854ab 100644
--- a/pylint/checkers/imports.py
+++ b/pylint/checkers/imports.py
@@ -15,6 +15,7 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 """imports checkers for Python code"""
 
+import os
 import sys
 from collections import defaultdict, Counter
 
@@ -22,7 +23,8 @@ import six
 
 import astroid
 from astroid import are_exclusive
-from astroid.modutils import get_module_part, is_standard_module
+from astroid.modutils import (EXT_LIB_DIR, get_module_part, is_standard_module,
+                              file_from_modpath)
 
 from pylint.interfaces import IAstroidChecker
 from pylint.utils import EmptyReport, get_global_option
@@ -178,6 +180,13 @@ MSGS = {
               'multiple-imports',
               'Used when import statement importing multiple modules is '
               'detected.'),
+    'C0411': ('%s comes before %s',
+              'wrong-import-order',
+              'Used when PEP8 import order is not observed (standard imports '
+              'first, then third-party libraries, then local imports)'),
+    'C0412': ('Imports from package %s are not grouped',
+              'ungrouped-imports',
+              'Used when imports are not grouped by packages'),
     }
 
 class ImportsChecker(BaseChecker):
@@ -227,11 +236,13 @@ given file (report RP0402 must not be disabled)'}
 given file (report RP0402 must not be disabled)'}
                ),
               )
+    ext_lib_dir = os.path.normcase(os.path.abspath(EXT_LIB_DIR))
 
     def __init__(self, linter=None):
         BaseChecker.__init__(self, linter)
         self.stats = None
         self.import_graph = None
+        self._imports_stack = []
         self.__int_dep_info = self.__ext_dep_info = None
         self.reports = (('RP0401', 'External dependencies',
                          self.report_external_dependencies),
@@ -265,6 +276,11 @@ 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):
+                importedname = importedmodnode.name if importedmodnode else None
+                if not importedname:
+                    importedname = node.names[0][0].split('.')[0]
+                self._imports_stack.append((node, importedname))
             if importedmodnode is None:
                 continue
             self._check_relative_import(modnode, node, importedmodnode, name)
@@ -292,6 +308,11 @@ given file (report RP0402 must not be disabled)'}
                 self.add_message('wildcard-import', args=basename, node=node)
         modnode = node.root()
         importedmodnode = self.get_imported_module(node, basename)
+        if isinstance(node.scope(), astroid.Module):
+            importedname = importedmodnode.name if importedmodnode else None
+            if not importedname:
+                importedname = node.names[0][0].split('.')[0]
+            self._imports_stack.append((node, importedname))
         if importedmodnode is None:
             return
         self._check_relative_import(modnode, node, importedmodnode, basename)
@@ -309,6 +330,67 @@ given file (report RP0402 must not be disabled)'}
                 self.add_message('reimported', node=node,
                                  args=(name, node.fromlineno))
 
+    @check_messages('wrong-import-order', 'ungrouped-imports')
+    def leave_module(self, node):
+        # check imports are grouped by category (standard, 3rd party, local)
+        std_imports, ext_imports, loc_imports = self._check_imports_order(node)
+        # check imports are grouped by package within a given category
+        for imports in (std_imports, ext_imports, loc_imports):
+            packages = []
+            for imp in imports:
+                if packages and packages[-1] == imp[1]:
+                    continue
+                # check if an import from the same package has already been made
+                for package in packages:
+                    if imp[1] == package:
+                        self.add_message('ungrouped-imports', node=imp[0],
+                                         args=package)
+                        break
+                packages.append(imp[1])
+        self._imports_stack = []
+
+    def _check_imports_order(self, node):
+        """Checks imports of module `node` are grouped by category
+
+        Imports must follow this order: standard, 3rd party, local
+        """
+        extern_imports = []
+        local_imports = []
+        std_imports = []
+        for node, modname in self._imports_stack:
+            if not modname:
+                local_imports.append((node, modname))
+                continue
+            package = modname.split('.')[0]
+            if is_standard_module(modname):
+                std_imports.append((node, package))
+                wrong_import = extern_imports or local_imports
+                if not wrong_import:
+                    continue
+                self.add_message('wrong-import-order', node=node,
+                                 args=('standard import "%s"' % node.as_string(),
+                                       '"%s"' % wrong_import[0][0].as_string()))
+            else:
+                try:
+                    filename = file_from_modpath([package])
+                except ImportError:
+                    local_imports.append((node, package))
+                    continue
+                if not filename:
+                    local_imports.append((node, package))
+                    continue
+                filename = os.path.normcase(os.path.abspath(filename))
+                if not filename.startswith(self.ext_lib_dir):
+                    local_imports.append((node, package))
+                    continue
+                extern_imports.append((node, package))
+                if not local_imports:
+                    continue
+                self.add_message('wrong-import-order', node=node,
+                                 args=('external import "%s"' % node.as_string(),
+                                       '"%s"' % local_imports[0][0].as_string()))
+        return std_imports, extern_imports, local_imports
+
     def get_imported_module(self, importnode, modname):
         try:
             return importnode.do_import_module(modname)
diff --git a/pylint/test/functional/class_members_py27.py b/pylint/test/functional/class_members_py27.py
index 415a890..97f2962 100644
--- a/pylint/test/functional/class_members_py27.py
+++ b/pylint/test/functional/class_members_py27.py
@@ -1,5 +1,5 @@
 """ Various tests for class members access. """

-# pylint: disable=R0903,print-statement,no-absolute-import, metaclass-assignment,import-error,no-init,missing-docstring

+# pylint: disable=R0903,print-statement,no-absolute-import, metaclass-assignment,import-error,no-init,missing-docstring, wrong-import-order

 from missing import Missing

 class MyClass(object):

     """class docstring"""

diff --git a/pylint/test/functional/deprecated_module_py2.py b/pylint/test/functional/deprecated_module_py2.py
index 399ed87..b0d1865 100644
--- a/pylint/test/functional/deprecated_module_py2.py
+++ b/pylint/test/functional/deprecated_module_py2.py
@@ -1,5 +1,5 @@
 """Test deprecated modules."""
-# pylint: disable=unused-import,no-name-in-module,import-error
+# pylint: disable=unused-import,no-name-in-module,import-error,ungrouped-imports
 
 import Bastion # [deprecated-module]
 import rexec # [deprecated-module]
diff --git a/pylint/test/functional/logging_format_interpolation.py b/pylint/test/functional/logging_format_interpolation.py
index 85117bf..5432d33 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
+# pylint: disable=E1101, no-absolute-import, import-error,line-too-long, missing-docstring,wrong-import-order
 
 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 ba2ee9c..712611f 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
+#pylint: disable=W0401,W0611,no-absolute-import,invalid-name,import-error,bare-except,broad-except,wrong-import-order,ungrouped-imports
 """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
new file mode 100644
index 0000000..9626062
--- /dev/null
+++ b/pylint/test/functional/ungrouped_imports.py
@@ -0,0 +1,20 @@
+"""Checks import order rule"""
+# pylint: disable=unused-import,relative-import,wrong-import-order,using-constant-test
+
+import six
+import logging.config
+import os.path
+from astroid import are_exclusive
+import logging  # [ungrouped-imports]
+import unused_import
+try:
+    import os  # [ungrouped-imports]
+except ImportError:
+    pass
+from os import pardir
+import scipy
+from os import sep
+import astroid  # [ungrouped-imports]
+if True:
+    import logging.handlers  # [ungrouped-imports]
+from os.path import join  # [ungrouped-imports]
diff --git a/pylint/test/functional/ungrouped_imports.txt b/pylint/test/functional/ungrouped_imports.txt
new file mode 100644
index 0000000..c29bb18
--- /dev/null
+++ b/pylint/test/functional/ungrouped_imports.txt
@@ -0,0 +1,5 @@
+ungrouped-imports:8::Imports from package logging are not grouped
+ungrouped-imports:11::Imports from package os are not grouped
+ungrouped-imports:17::Imports from package astroid are not grouped
+ungrouped-imports:19::Imports from package logging are not grouped
+ungrouped-imports:20::Imports from package os are not grouped
diff --git a/pylint/test/functional/unpacking_non_sequence.py b/pylint/test/functional/unpacking_non_sequence.py
index 1e5de23..c03b63d 100644
--- a/pylint/test/functional/unpacking_non_sequence.py
+++ b/pylint/test/functional/unpacking_non_sequence.py
@@ -3,8 +3,8 @@
 # pylint: disable=too-few-public-methods, invalid-name, attribute-defined-outside-init, unused-variable, no-absolute-import
 # pylint: disable=using-constant-test, no-init
 from os import rename as nonseq_func
-from functional.unpacking import nonseq
 from six import with_metaclass
+from functional.unpacking import nonseq
 
 __revision__ = 0
 
diff --git a/pylint/test/functional/unsubscriptable_value.py b/pylint/test/functional/unsubscriptable_value.py
index 221bd17..64cafaf 100644
--- a/pylint/test/functional/unsubscriptable_value.py
+++ b/pylint/test/functional/unsubscriptable_value.py
@@ -3,7 +3,7 @@ Checks that value used in a subscript supports subscription
 (i.e. defines __getitem__ method).
 """
 # pylint: disable=missing-docstring,pointless-statement,expression-not-assigned
-# pylint: disable=too-few-public-methods,import-error,invalid-name
+# pylint: disable=too-few-public-methods,import-error,invalid-name,wrong-import-order
 import six
 
 # primitives
diff --git a/pylint/test/functional/wrong_import_order.py b/pylint/test/functional/wrong_import_order.py
new file mode 100644
index 0000000..9fc3996
--- /dev/null
+++ b/pylint/test/functional/wrong_import_order.py
@@ -0,0 +1,11 @@
+"""Checks import order rule"""
+# pylint: disable=unused-import,relative-import,ungrouped-imports
+
+import six
+import os.path  # [wrong-import-order]
+from astroid import are_exclusive
+import sys  # [wrong-import-order]
+import datetime  # [wrong-import-order]
+import unused_import
+import scipy  # [wrong-import-order]
+import astroid
diff --git a/pylint/test/functional/wrong_import_order.txt b/pylint/test/functional/wrong_import_order.txt
new file mode 100644
index 0000000..537355c
--- /dev/null
+++ b/pylint/test/functional/wrong_import_order.txt
@@ -0,0 +1,4 @@
+wrong-import-order:5::standard import "import os.path" comes before "import six"
+wrong-import-order:7::standard import "import sys" comes before "import six"
+wrong-import-order:8::standard import "import datetime" comes before "import six"
+wrong-import-order:10::external import "import scipy" comes before "from astroid import are_exclusive"
diff --git a/pylint/test/input/func_w0404.py b/pylint/test/input/func_w0404.py
index d65da51..b9169d7 100644
--- a/pylint/test/input/func_w0404.py
+++ b/pylint/test/input/func_w0404.py
@@ -10,7 +10,7 @@ from xml.etree import ElementTree
 from email import encoders
 import email.encoders
 
-import sys
+import sys  #pylint: disable=ungrouped-imports
 
 def no_reimport():
     """docstring"""
diff --git a/pylint/test/input/func_w0405.py b/pylint/test/input/func_w0405.py
index f1c1677..3f6efc7 100644
--- a/pylint/test/input/func_w0405.py
+++ b/pylint/test/input/func_w0405.py
@@ -2,7 +2,7 @@
 """
 from __future__ import absolute_import, print_function
 __revision__ = 0
-# pylint: disable=using-constant-test
+# pylint: disable=using-constant-test,ungrouped-imports
 import os
 from os.path import join, exists