From 0b5ab33c81a753ad732bfa5cdcf933bb2167a12c Mon Sep 17 00:00:00 2001 From: Claudiu Popa Date: Tue, 1 Dec 2015 10:33:33 +0200 Subject: 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. --- pylint/checkers/imports.py | 145 +++++++++++++++++++--------------- pylint/test/functional/reimported.py | 7 ++ pylint/test/functional/reimported.txt | 3 + pylint/test/test_import_graph.py | 4 +- 4 files changed, 92 insertions(+), 67 deletions(-) create mode 100644 pylint/test/functional/reimported.py create mode 100644 pylint/test/functional/reimported.txt 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.] 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(), ''' -- cgit v1.2.1