Refactor things through the imports checker

This patch transforms some public functions / methods to private
and moves some blocks of code into their own functions. Through
the latter, a couple of new messages are now emitted even though
the module couldn't be imported, such as reimported, which doesn't
make sense to not emit in this case.

4 files changed, 92 insertions, 67 deletions

diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py
index cca4998..a2f381c 100644
--- a/pylint/checkers/imports.py
+++ b/pylint/checkers/imports.py
@@ -15,9 +15,9 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 """imports checkers for Python code"""
 
+import collections
 import os
 import sys
-from collections import defaultdict, Counter
 
 import six
 
@@ -34,6 +34,18 @@ from pylint.graph import get_cycles, DotBackend
 from pylint.reporters.ureports.nodes import VerbatimText, Paragraph
 
 
+def _qualified_names(modname):
+    """Split the names of the given module into subparts
+
+    For example,
+        _qualified_names('pylint.checkers.ImportsChecker')
+    returns
+        ['pylint', 'pylint.checkers', 'pylint.checkers.ImportsChecker']
+    """
+    names = modname.split('.')
+    return ['.'.join(names[0:i+1]) for i in range(len(names))]
+
+
 def _get_import_name(importnode, modname):
     """Get a prepared module name from the given import node
 
@@ -51,7 +63,7 @@ def _get_import_name(importnode, modname):
     return modname
 
 
-def get_first_import(node, context, name, base, level):
+def _get_first_import(node, context, name, base, level):
     """return the node where [base.]<name> is imported or None if not found
     """
     fullname = '%s.%s' % (base, name) if base else name
@@ -78,7 +90,7 @@ def get_first_import(node, context, name, base, level):
 
 # utilities to represents import dependencies as tree and dot graph ###########
 
-def make_tree_defs(mod_files_list):
+def _make_tree_defs(mod_files_list):
     """get a list of 2-uple (module, list_of_files_which_import_this_module),
     it will return a dictionary to represent this as a tree
     """
@@ -90,7 +102,8 @@ def make_tree_defs(mod_files_list):
         node[1] += files
     return tree_defs
 
-def repr_tree_defs(data, indent_str=None):
+
+def _repr_tree_defs(data, indent_str=None):
     """return a string which represents imports as a tree"""
     lines = []
     nodes = data.items()
@@ -109,11 +122,11 @@ def repr_tree_defs(data, indent_str=None):
             else:
                 sub_indent_str = '%s| ' % indent_str
         if sub:
-            lines.append(repr_tree_defs(sub, sub_indent_str))
+            lines.append(_repr_tree_defs(sub, sub_indent_str))
     return '\n'.join(lines)
 
 
-def dependencies_graph(filename, dep_info):
+def _dependencies_graph(filename, dep_info):
     """write dependencies as a dot (graphviz) file
     """
     done = {}
@@ -132,11 +145,11 @@ def dependencies_graph(filename, dep_info):
     printer.generate(filename)
 
 
-def make_graph(filename, dep_info, sect, gtype):
+def _make_graph(filename, dep_info, sect, gtype):
     """generate a dependencies graph and add some information about it in the
     report's section
     """
-    dependencies_graph(filename, dep_info)
+    _dependencies_graph(filename, dep_info)
     sect.append(Paragraph('%simports graph has been written to %s'
                           % (gtype, filename)))
 
@@ -207,7 +220,7 @@ class ImportsChecker(BaseChecker):
     msgs = MSGS
     priority = -2
 
-    if sys.version_info < (3,):
+    if six.PY2:
         deprecated_modules = ('regsub', 'TERMIOS', 'Bastion', 'rexec')
     else:
         deprecated_modules = ('optparse', )
@@ -250,9 +263,9 @@ given file (report RP0402 must not be disabled)'}
         self._first_non_import_node = None
         self.__int_dep_info = self.__ext_dep_info = None
         self.reports = (('RP0401', 'External dependencies',
-                         self.report_external_dependencies),
+                         self._report_external_dependencies),
                         ('RP0402', 'Modules dependencies graph',
-                         self.report_dependencies_graph),
+                         self._report_dependencies_graph),
                        )
 
     def open(self):
@@ -260,7 +273,7 @@ given file (report RP0402 must not be disabled)'}
         self.linter.add_stats(dependencies={})
         self.linter.add_stats(cycles=[])
         self.stats = self.linter.stats
-        self.import_graph = defaultdict(set)
+        self.import_graph = collections.defaultdict(set)
         self._ignored_modules = get_global_option(
             self, 'ignored-modules', default=[])
 
@@ -272,44 +285,40 @@ given file (report RP0402 must not be disabled)'}
             for cycle in get_cycles(self.import_graph, vertices=vertices):
                 self.add_message('cyclic-import', args=' -> '.join(cycle))
 
-    @check_messages('wrong-import-position')
+    @check_messages('wrong-import-position', 'multiple-imports',
+                    'relative-import', 'reimported')
     def visit_import(self, node):
         """triggered when an import statement is seen"""
+        self._check_reimport(node)
+
         modnode = node.root()
         names = [name for name, _ in node.names]
         if len(names) >= 2:
             self.add_message('multiple-imports', args=', '.join(names), node=node)
+
         for name in names:
             self._check_deprecated_module(node, name)
             importedmodnode = self.get_imported_module(node, name)
             if isinstance(node.scope(), astroid.Module):
                 self._check_position(node)
                 self._record_import(node, importedmodnode)
+
             if importedmodnode is None:
                 continue
+
             self._check_relative_import(modnode, node, importedmodnode, name)
             self._add_imported_module(node, importedmodnode.name)
-            self._check_reimport(node, name)
 
-    # TODO This appears to be the list of all messages of the checker...
-    # @check_messages('W0410', 'W0401', 'W0403', 'W0402', 'W0404', 'W0406', 'E0401')
     @check_messages(*(MSGS.keys()))
     def visit_importfrom(self, node):
         """triggered when a from statement is seen"""
         basename = node.modname
-        if basename == '__future__':
-            # check if this is the first non-docstring statement in the module
-            prev = node.previous_sibling()
-            if prev:
-                # consecutive future statements are possible
-                if not (isinstance(prev, astroid.ImportFrom)
-                        and prev.modname == '__future__'):
-                    self.add_message('misplaced-future', node=node)
-            return
+        self._check_misplaced_future(node)
         self._check_deprecated_module(node, basename)
-        for name, _ in node.names:
-            if name == '*':
-                self.add_message('wildcard-import', args=basename, node=node)
+        self._check_wildcard_imports(node)
+        self._check_same_line_imports(node)
+        self._check_reimport(node, basename=basename, level=node.level)
+
         modnode = node.root()
         importedmodnode = self.get_imported_module(node, basename)
         if isinstance(node.scope(), astroid.Module):
@@ -322,15 +331,6 @@ given file (report RP0402 must not be disabled)'}
         for name, _ in node.names:
             if name != '*':
                 self._add_imported_module(node, '%s.%s' % (importedmodnode.name, name))
-                self._check_reimport(node, name, basename, node.level)
-
-        # Detect duplicate imports on the same line.
-        names = (name for name, _ in node.names)
-        counter = Counter(names)
-        for name, count in counter.items():
-            if count > 1:
-                self.add_message('reimported', node=node,
-                                 args=(name, node.fromlineno))
 
     @check_messages('wrong-import-order', 'ungrouped-imports',
                     'wrong-import-position')
@@ -375,6 +375,27 @@ given file (report RP0402 must not be disabled)'}
 
     visit_classdef = visit_for = visit_while = visit_functiondef
 
+    def _check_misplaced_future(self, node):
+        basename = node.modname
+        if basename == '__future__':
+            # check if this is the first non-docstring statement in the module
+            prev = node.previous_sibling()
+            if prev:
+                # consecutive future statements are possible
+                if not (isinstance(prev, astroid.ImportFrom)
+                        and prev.modname == '__future__'):
+                    self.add_message('misplaced-future', node=node)
+            return
+
+    def _check_same_line_imports(self, node):
+        # Detect duplicate imports on the same line.
+        names = (name for name, _ in node.names)
+        counter = collections.Counter(names)
+        for name, count in counter.items():
+            if count > 1:
+                self.add_message('reimported', node=node,
+                                 args=(name, node.fromlineno))
+
     def _check_position(self, node):
         """Check `node` import or importfrom node position is correct
 
@@ -447,25 +468,13 @@ given file (report RP0402 must not be disabled)'}
             else:
                 args = repr(dotted_modname)
 
-            for submodule in self._qualified_names(modname):
+            for submodule in _qualified_names(modname):
                 if submodule in self._ignored_modules:
                     return None
 
             if not node_ignores_exception(importnode, ImportError):
                 self.add_message("import-error", args=args, node=importnode)
 
-    @staticmethod
-    def _qualified_names(modname):
-        """Split the names of the given module into subparts
-
-        For example,
-            _qualified_names('pylint.checkers.ImportsChecker')
-        returns
-            ['pylint', 'pylint.checkers', 'pylint.checkers.ImportsChecker']
-        """
-        names = modname.split('.')
-        return ['.'.join(names[0:i+1]) for i in range(len(names))]
-
     def _check_relative_import(self, modnode, importnode, importedmodnode,
                                importedasname):
         """check relative import. node is either an Import or From node, modname
@@ -513,7 +522,7 @@ given file (report RP0402 must not be disabled)'}
             if mod_path == mod_name or mod_path.startswith(mod_name + '.'):
                 self.add_message('deprecated-module', node=node, args=mod_path)
 
-    def _check_reimport(self, node, name, basename=None, level=None):
+    def _check_reimport(self, node, basename=None, level=None):
         """check if the import is necessary (i.e. not already done)"""
         if not self.linter.is_message_enabled('reimported'):
             return
@@ -523,22 +532,23 @@ given file (report RP0402 must not be disabled)'}
         contexts = [(frame, level)]
         if root is not frame:
             contexts.append((root, None))
-        for context, level in contexts:
-            first = get_first_import(node, context, name, basename, level)
-            if first is not None:
-                self.add_message('reimported', node=node,
-                                 args=(name, first.fromlineno))
 
+        for context, level in contexts:
+            for name, _ in node.names:
+                first = _get_first_import(node, context, name, basename, level)
+                if first is not None:
+                    self.add_message('reimported', node=node,
+                                     args=(name, first.fromlineno))
 
-    def report_external_dependencies(self, sect, _, dummy):
+    def _report_external_dependencies(self, sect, _, dummy):
         """return a verbatim layout for displaying dependencies"""
-        dep_info = make_tree_defs(six.iteritems(self._external_dependencies_info()))
+        dep_info = _make_tree_defs(six.iteritems(self._external_dependencies_info()))
         if not dep_info:
             raise EmptyReport()
-        tree_str = repr_tree_defs(dep_info)
+        tree_str = _repr_tree_defs(dep_info)
         sect.append(VerbatimText(tree_str))
 
-    def report_dependencies_graph(self, sect, _, dummy):
+    def _report_dependencies_graph(self, sect, _, dummy):
         """write dependencies as a dot (graphviz) file"""
         dep_info = self.stats['dependencies']
         if not dep_info or not (self.config.import_graph
@@ -547,15 +557,15 @@ given file (report RP0402 must not be disabled)'}
             raise EmptyReport()
         filename = self.config.import_graph
         if filename:
-            make_graph(filename, dep_info, sect, '')
+            _make_graph(filename, dep_info, sect, '')
         filename = self.config.ext_import_graph
         if filename:
-            make_graph(filename, self._external_dependencies_info(),
-                       sect, 'external ')
+            _make_graph(filename, self._external_dependencies_info(),
+                        sect, 'external ')
         filename = self.config.int_import_graph
         if filename:
-            make_graph(filename, self._internal_dependencies_info(),
-                       sect, 'internal ')
+            _make_graph(filename, self._internal_dependencies_info(),
+                        sect, 'internal ')
 
     def _external_dependencies_info(self):
         """return cached external dependencies information or build and
@@ -581,6 +591,11 @@ given file (report RP0402 must not be disabled)'}
                     result[importee] = importers
         return self.__int_dep_info
 
+    def _check_wildcard_imports(self, node):
+        for name, _ in node.names:
+            if name == '*':
+                self.add_message('wildcard-import', args=node.modname, node=node)
+
 
 def register(linter):
     """required method to auto register this checker """
diff --git a/pylint/test/functional/reimported.py b/pylint/test/functional/reimported.py
new file mode 100644
index 0000000..1025832
--- /dev/null
+++ b/pylint/test/functional/reimported.py
@@ -0,0 +1,7 @@
+# pylint: disable=missing-docstring,unused-import,import-error

+

+from time import sleep, sleep # [reimported]

+from lala import missing, missing # [reimported]

+

+import missing1

+import missing1 # [reimported]

diff --git a/pylint/test/functional/reimported.txt b/pylint/test/functional/reimported.txt
new file mode 100644
index 0000000..685c83d
--- /dev/null
+++ b/pylint/test/functional/reimported.txt
@@ -0,0 +1,3 @@
+reimported:3::"Reimport 'sleep' (imported line 3)"
+reimported:4::"Reimport 'missing' (imported line 4)"
+reimported:7::"Reimport 'missing1' (imported line 6)"
\ No newline at end of file
diff --git a/pylint/test/test_import_graph.py b/pylint/test/test_import_graph.py
index 2b41536..edef0db 100644
--- a/pylint/test/test_import_graph.py
+++ b/pylint/test/test_import_graph.py
@@ -18,8 +18,8 @@ class DependenciesGraphTC(unittest.TestCase):
         os.remove(self.dest)
 
     def test_dependencies_graph(self):
-        imports.dependencies_graph(self.dest, {'labas': ['hoho', 'yep'],
-                                               'hoho': ['yep']})
+        imports._dependencies_graph(self.dest, {'labas': ['hoho', 'yep'],
+                                                'hoho': ['yep']})
         with open(self.dest) as stream:
             self.assertEqual(stream.read().strip(),
                           '''