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.

authorClaudiu Popa <pcmanticore@gmail.com>
changeset87291a9b9f82
branchdefault
phasepublic
hiddenno
parent revision#67a5812decfc Add else-if-used rule functional test
child revision#3f58367d04fd Don't emit import-self and cyclic-import for relative imports of modules with the same name as the package itself.
files modified by this revision
pylint/checkers/imports.py
pylint/test/functional/reimported.py
pylint/test/functional/reimported.txt
pylint/test/test_import_graph.py
# HG changeset patch
# User Claudiu Popa <pcmanticore@gmail.com>
# Date 1448958813 -7200
# Tue Dec 01 10:33:33 2015 +0200
# Node ID 87291a9b9f821772e68bce9687e83680fe9ce430
# Parent 67a5812decfc34ef7a50a0b030a7bbc0096a1f96
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.

diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py
@@ -13,13 +13,13 @@
1  # You should have received a copy of the GNU General Public License along with
2  # this program; if not, write to the Free Software Foundation, Inc.,
3  # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
4  """imports checkers for Python code"""
5 
6 +import collections
7  import os
8  import sys
9 -from collections import defaultdict, Counter
10 
11  import six
12 
13  import astroid
14  from astroid import are_exclusive
@@ -32,10 +32,22 @@
15  from pylint.checkers.utils import check_messages, node_ignores_exception
16  from pylint.graph import get_cycles, DotBackend
17  from pylint.reporters.ureports.nodes import VerbatimText, Paragraph
18 
19 
20 +def _qualified_names(modname):
21 +    """Split the names of the given module into subparts
22 +
23 +    For example,
24 +        _qualified_names('pylint.checkers.ImportsChecker')
25 +    returns
26 +        ['pylint', 'pylint.checkers', 'pylint.checkers.ImportsChecker']
27 +    """
28 +    names = modname.split('.')
29 +    return ['.'.join(names[0:i+1]) for i in range(len(names))]
30 +
31 +
32  def _get_import_name(importnode, modname):
33      """Get a prepared module name from the given import node
34 
35      In the case of relative imports, this will return the
36      absolute qualified module name, which might be useful
@@ -49,11 +61,11 @@
37                  modname = root.relative_to_absolute_name(
38                      modname, level=importnode.level)
39      return modname
40 
41 
42 -def get_first_import(node, context, name, base, level):
43 +def _get_first_import(node, context, name, base, level):
44      """return the node where [base.]<name> is imported or None if not found
45      """
46      fullname = '%s.%s' % (base, name) if base else name
47 
48      first = None
@@ -76,11 +88,11 @@
49      if found and not are_exclusive(first, node):
50          return first
51 
52  # utilities to represents import dependencies as tree and dot graph ###########
53 
54 -def make_tree_defs(mod_files_list):
55 +def _make_tree_defs(mod_files_list):
56      """get a list of 2-uple (module, list_of_files_which_import_this_module),
57      it will return a dictionary to represent this as a tree
58      """
59      tree_defs = {}
60      for mod, files in mod_files_list:
@@ -88,11 +100,12 @@
61          for prefix in mod.split('.'):
62              node = node[0].setdefault(prefix, [{}, []])
63          node[1] += files
64      return tree_defs
65 
66 -def repr_tree_defs(data, indent_str=None):
67 +
68 +def _repr_tree_defs(data, indent_str=None):
69      """return a string which represents imports as a tree"""
70      lines = []
71      nodes = data.items()
72      for i, (mod, (sub, files)) in enumerate(sorted(nodes, key=lambda x: x[0])):
73          if not files:
@@ -107,15 +120,15 @@
74              if i == len(nodes)-1:
75                  sub_indent_str = '%s  ' % indent_str
76              else:
77                  sub_indent_str = '%s| ' % indent_str
78          if sub:
79 -            lines.append(repr_tree_defs(sub, sub_indent_str))
80 +            lines.append(_repr_tree_defs(sub, sub_indent_str))
81      return '\n'.join(lines)
82 
83 
84 -def dependencies_graph(filename, dep_info):
85 +def _dependencies_graph(filename, dep_info):
86      """write dependencies as a dot (graphviz) file
87      """
88      done = {}
89      printer = DotBackend(filename[:-4], rankdir='LR')
90      printer.emit('URL="." node[shape="box"]')
@@ -130,15 +143,15 @@
91          for modname in dependencies:
92              printer.emit_edge(modname, depmodname)
93      printer.generate(filename)
94 
95 
96 -def make_graph(filename, dep_info, sect, gtype):
97 +def _make_graph(filename, dep_info, sect, gtype):
98      """generate a dependencies graph and add some information about it in the
99      report's section
100      """
101 -    dependencies_graph(filename, dep_info)
102 +    _dependencies_graph(filename, dep_info)
103      sect.append(Paragraph('%simports graph has been written to %s'
104                            % (gtype, filename)))
105 
106 
107  # the import checker itself ###################################################
@@ -205,11 +218,11 @@
108 
109      name = 'imports'
110      msgs = MSGS
111      priority = -2
112 
113 -    if sys.version_info < (3,):
114 +    if six.PY2:
115          deprecated_modules = ('regsub', 'TERMIOS', 'Bastion', 'rexec')
116      else:
117          deprecated_modules = ('optparse', )
118      options = (('deprecated-modules',
119                  {'default' : deprecated_modules,
@@ -248,21 +261,21 @@
120          self.import_graph = None
121          self._imports_stack = []
122          self._first_non_import_node = None
123          self.__int_dep_info = self.__ext_dep_info = None
124          self.reports = (('RP0401', 'External dependencies',
125 -                         self.report_external_dependencies),
126 +                         self._report_external_dependencies),
127                          ('RP0402', 'Modules dependencies graph',
128 -                         self.report_dependencies_graph),
129 +                         self._report_dependencies_graph),
130                         )
131 
132      def open(self):
133          """called before visiting project (i.e set of modules)"""
134          self.linter.add_stats(dependencies={})
135          self.linter.add_stats(cycles=[])
136          self.stats = self.linter.stats
137 -        self.import_graph = defaultdict(set)
138 +        self.import_graph = collections.defaultdict(set)
139          self._ignored_modules = get_global_option(
140              self, 'ignored-modules', default=[])
141 
142      def close(self):
143          """called before visiting project (i.e set of modules)"""
@@ -270,48 +283,44 @@
144          if self.linter.is_message_enabled('cyclic-import'):
145              vertices = list(self.import_graph)
146              for cycle in get_cycles(self.import_graph, vertices=vertices):
147                  self.add_message('cyclic-import', args=' -> '.join(cycle))
148 
149 -    @check_messages('wrong-import-position')
150 +    @check_messages('wrong-import-position', 'multiple-imports',
151 +                    'relative-import', 'reimported')
152      def visit_import(self, node):
153          """triggered when an import statement is seen"""
154 +        self._check_reimport(node)
155 +
156          modnode = node.root()
157          names = [name for name, _ in node.names]
158          if len(names) >= 2:
159              self.add_message('multiple-imports', args=', '.join(names), node=node)
160 +
161          for name in names:
162              self._check_deprecated_module(node, name)
163              importedmodnode = self.get_imported_module(node, name)
164              if isinstance(node.scope(), astroid.Module):
165                  self._check_position(node)
166                  self._record_import(node, importedmodnode)
167 +
168              if importedmodnode is None:
169                  continue
170 +
171              self._check_relative_import(modnode, node, importedmodnode, name)
172              self._add_imported_module(node, importedmodnode.name)
173 -            self._check_reimport(node, name)
174 
175 -    # TODO This appears to be the list of all messages of the checker...
176 -    # @check_messages('W0410', 'W0401', 'W0403', 'W0402', 'W0404', 'W0406', 'E0401')
177      @check_messages(*(MSGS.keys()))
178      def visit_importfrom(self, node):
179          """triggered when a from statement is seen"""
180          basename = node.modname
181 -        if basename == '__future__':
182 -            # check if this is the first non-docstring statement in the module
183 -            prev = node.previous_sibling()
184 -            if prev:
185 -                # consecutive future statements are possible
186 -                if not (isinstance(prev, astroid.ImportFrom)
187 -                        and prev.modname == '__future__'):
188 -                    self.add_message('misplaced-future', node=node)
189 -            return
190 +        self._check_misplaced_future(node)
191          self._check_deprecated_module(node, basename)
192 -        for name, _ in node.names:
193 -            if name == '*':
194 -                self.add_message('wildcard-import', args=basename, node=node)
195 +        self._check_wildcard_imports(node)
196 +        self._check_same_line_imports(node)
197 +        self._check_reimport(node, basename=basename, level=node.level)
198 +
199          modnode = node.root()
200          importedmodnode = self.get_imported_module(node, basename)
201          if isinstance(node.scope(), astroid.Module):
202              self._check_position(node)
203              self._record_import(node, importedmodnode)
@@ -320,19 +329,10 @@
204          self._check_relative_import(modnode, node, importedmodnode, basename)
205 
206          for name, _ in node.names:
207              if name != '*':
208                  self._add_imported_module(node, '%s.%s' % (importedmodnode.name, name))
209 -                self._check_reimport(node, name, basename, node.level)
210 -
211 -        # Detect duplicate imports on the same line.
212 -        names = (name for name, _ in node.names)
213 -        counter = Counter(names)
214 -        for name, count in counter.items():
215 -            if count > 1:
216 -                self.add_message('reimported', node=node,
217 -                                 args=(name, node.fromlineno))
218 
219      @check_messages('wrong-import-order', 'ungrouped-imports',
220                      'wrong-import-position')
221      def leave_module(self, node):
222          # Check imports are grouped by category (standard, 3rd party, local)
@@ -373,10 +373,31 @@
223          if not self._first_non_import_node:
224              self._first_non_import_node = node
225 
226      visit_classdef = visit_for = visit_while = visit_functiondef
227 
228 +    def _check_misplaced_future(self, node):
229 +        basename = node.modname
230 +        if basename == '__future__':
231 +            # check if this is the first non-docstring statement in the module
232 +            prev = node.previous_sibling()
233 +            if prev:
234 +                # consecutive future statements are possible
235 +                if not (isinstance(prev, astroid.ImportFrom)
236 +                        and prev.modname == '__future__'):
237 +                    self.add_message('misplaced-future', node=node)
238 +            return
239 +
240 +    def _check_same_line_imports(self, node):
241 +        # Detect duplicate imports on the same line.
242 +        names = (name for name, _ in node.names)
243 +        counter = collections.Counter(names)
244 +        for name, count in counter.items():
245 +            if count > 1:
246 +                self.add_message('reimported', node=node,
247 +                                 args=(name, node.fromlineno))
248 +
249      def _check_position(self, node):
250          """Check `node` import or importfrom node position is correct
251 
252          Send a message  if `node` comes before another instruction
253          """
@@ -445,29 +466,17 @@
254              if str(ex) != modname:
255                  args = '%r (%s)' % (dotted_modname, ex)
256              else:
257                  args = repr(dotted_modname)
258 
259 -            for submodule in self._qualified_names(modname):
260 +            for submodule in _qualified_names(modname):
261                  if submodule in self._ignored_modules:
262                      return None
263 
264              if not node_ignores_exception(importnode, ImportError):
265                  self.add_message("import-error", args=args, node=importnode)
266 
267 -    @staticmethod
268 -    def _qualified_names(modname):
269 -        """Split the names of the given module into subparts
270 -
271 -        For example,
272 -            _qualified_names('pylint.checkers.ImportsChecker')
273 -        returns
274 -            ['pylint', 'pylint.checkers', 'pylint.checkers.ImportsChecker']
275 -        """
276 -        names = modname.split('.')
277 -        return ['.'.join(names[0:i+1]) for i in range(len(names))]
278 -
279      def _check_relative_import(self, modnode, importnode, importedmodnode,
280                                 importedasname):
281          """check relative import. node is either an Import or From node, modname
282          the imported module name.
283          """
@@ -511,53 +520,54 @@
284          """check if the module is deprecated"""
285          for mod_name in self.config.deprecated_modules:
286              if mod_path == mod_name or mod_path.startswith(mod_name + '.'):
287                  self.add_message('deprecated-module', node=node, args=mod_path)
288 
289 -    def _check_reimport(self, node, name, basename=None, level=None):
290 +    def _check_reimport(self, node, basename=None, level=None):
291          """check if the import is necessary (i.e. not already done)"""
292          if not self.linter.is_message_enabled('reimported'):
293              return
294 
295          frame = node.frame()
296          root = node.root()
297          contexts = [(frame, level)]
298          if root is not frame:
299              contexts.append((root, None))
300 +
301          for context, level in contexts:
302 -            first = get_first_import(node, context, name, basename, level)
303 -            if first is not None:
304 -                self.add_message('reimported', node=node,
305 -                                 args=(name, first.fromlineno))
306 +            for name, _ in node.names:
307 +                first = _get_first_import(node, context, name, basename, level)
308 +                if first is not None:
309 +                    self.add_message('reimported', node=node,
310 +                                     args=(name, first.fromlineno))
311 
312 -
313 -    def report_external_dependencies(self, sect, _, dummy):
314 +    def _report_external_dependencies(self, sect, _, dummy):
315          """return a verbatim layout for displaying dependencies"""
316 -        dep_info = make_tree_defs(six.iteritems(self._external_dependencies_info()))
317 +        dep_info = _make_tree_defs(six.iteritems(self._external_dependencies_info()))
318          if not dep_info:
319              raise EmptyReport()
320 -        tree_str = repr_tree_defs(dep_info)
321 +        tree_str = _repr_tree_defs(dep_info)
322          sect.append(VerbatimText(tree_str))
323 
324 -    def report_dependencies_graph(self, sect, _, dummy):
325 +    def _report_dependencies_graph(self, sect, _, dummy):
326          """write dependencies as a dot (graphviz) file"""
327          dep_info = self.stats['dependencies']
328          if not dep_info or not (self.config.import_graph
329                                  or self.config.ext_import_graph
330                                  or self.config.int_import_graph):
331              raise EmptyReport()
332          filename = self.config.import_graph
333          if filename:
334 -            make_graph(filename, dep_info, sect, '')
335 +            _make_graph(filename, dep_info, sect, '')
336          filename = self.config.ext_import_graph
337          if filename:
338 -            make_graph(filename, self._external_dependencies_info(),
339 -                       sect, 'external ')
340 +            _make_graph(filename, self._external_dependencies_info(),
341 +                        sect, 'external ')
342          filename = self.config.int_import_graph
343          if filename:
344 -            make_graph(filename, self._internal_dependencies_info(),
345 -                       sect, 'internal ')
346 +            _make_graph(filename, self._internal_dependencies_info(),
347 +                        sect, 'internal ')
348 
349      def _external_dependencies_info(self):
350          """return cached external dependencies information or build and
351          cache them
352          """
@@ -579,9 +589,14 @@
353              for importee, importers in six.iteritems(self.stats['dependencies']):
354                  if importee.startswith(package):
355                      result[importee] = importers
356          return self.__int_dep_info
357 
358 +    def _check_wildcard_imports(self, node):
359 +        for name, _ in node.names:
360 +            if name == '*':
361 +                self.add_message('wildcard-import', args=node.modname, node=node)
362 +
363 
364  def register(linter):
365      """required method to auto register this checker """
366      linter.register_checker(ImportsChecker(linter))
diff --git a/pylint/test/functional/reimported.py b/pylint/test/functional/reimported.py
@@ -0,0 +1,7 @@
367 +# pylint: disable=missing-docstring,unused-import,import-error

368 +

369 +from time import sleep, sleep # [reimported]

370 +from lala import missing, missing # [reimported]

371 +

372 +import missing1

373 +import missing1 # [reimported]

diff --git a/pylint/test/functional/reimported.txt b/pylint/test/functional/reimported.txt
@@ -0,0 +1,3 @@
374 +reimported:3::"Reimport 'sleep' (imported line 3)"
375 +reimported:4::"Reimport 'missing' (imported line 4)"
376 +reimported:7::"Reimport 'missing1' (imported line 6)"
377 \ No newline at end of file
diff --git a/pylint/test/test_import_graph.py b/pylint/test/test_import_graph.py
@@ -16,12 +16,12 @@
378      dest = 'dependencies_graph.dot'
379      def tearDown(self):
380          os.remove(self.dest)
381 
382      def test_dependencies_graph(self):
383 -        imports.dependencies_graph(self.dest, {'labas': ['hoho', 'yep'],
384 -                                               'hoho': ['yep']})
385 +        imports._dependencies_graph(self.dest, {'labas': ['hoho', 'yep'],
386 +                                                'hoho': ['yep']})
387          with open(self.dest) as stream:
388              self.assertEqual(stream.read().strip(),
389                            '''
390  digraph "dependencies_graph" {
391  rankdir=LR