diff -r ab2c5163900e -r 1a6a117d0b95 contrib/import-checker.py --- a/contrib/import-checker.py Sun Jun 28 12:28:48 2015 -0700 +++ b/contrib/import-checker.py Sun Jun 28 12:46:34 2015 -0700 @@ -8,6 +8,33 @@ import BaseHTTPServer import zlib +# Whitelist of modules that symbols can be directly imported from. +allowsymbolimports = ( + '__future__', + 'mercurial.i18n', + 'mercurial.node', +) + +# Modules that must be aliased because they are commonly confused with +# common variables and can create aliasing and readability issues. +requirealias = { + 'ui': 'uimod', +} + +def usingabsolute(root): + """Whether absolute imports are being used.""" + if sys.version_info[0] >= 3: + return True + + for node in ast.walk(root): + if isinstance(node, ast.ImportFrom): + if node.module == '__future__': + for n in node.names: + if n.name == 'absolute_import': + return True + + return False + def dotted_name_of_path(path, trimpure=False): """Given a relative path to a source file, return its dotted module name. @@ -273,10 +300,157 @@ yield found[1] def verify_import_convention(module, source): - """Verify imports match our established coding convention.""" + """Verify imports match our established coding convention. + + We have 2 conventions: legacy and modern. The modern convention is in + effect when using absolute imports. + + The legacy convention only looks for mixed imports. The modern convention + is much more thorough. + """ root = ast.parse(source) + absolute = usingabsolute(root) - return verify_stdlib_on_own_line(root) + if absolute: + return verify_modern_convention(module, root) + else: + return verify_stdlib_on_own_line(root) + +def verify_modern_convention(module, root): + """Verify a file conforms to the modern import convention rules. + + The rules of the modern convention are: + + * Ordering is stdlib followed by local imports. Each group is lexically + sorted. + * Importing multiple modules via "import X, Y" is not allowed: use + separate import statements. + * Importing multiple modules via "from X import ..." is allowed if using + parenthesis and one entry per line. + * Only 1 relative import statement per import level ("from .", "from ..") + is allowed. + * Relative imports from higher levels must occur before lower levels. e.g. + "from .." must be before "from .". + * Imports from peer packages should use relative import (e.g. do not + "import mercurial.foo" from a "mercurial.*" module). + * Symbols can only be imported from specific modules (see + `allowsymbolimports`). For other modules, first import the module then + assign the symbol to a module-level variable. In addition, these imports + must be performed before other relative imports. This rule only + applies to import statements outside of any blocks. + * Relative imports from the standard library are not allowed. + * Certain modules must be aliased to alternate names to avoid aliasing + and readability problems. See `requirealias`. + """ + topmodule = module.split('.')[0] + + # Whether a local/non-stdlib import has been performed. + seenlocal = False + # Whether a relative, non-symbol import has been seen. + seennonsymbolrelative = False + # The last name to be imported (for sorting). + lastname = None + # Relative import levels encountered so far. + seenlevels = set() + + for node in ast.walk(root): + if isinstance(node, ast.Import): + # Disallow "import foo, bar" and require separate imports + # for each module. + if len(node.names) > 1: + yield 'multiple imported names: %s' % ', '.join( + n.name for n in node.names) + + name = node.names[0].name + asname = node.names[0].asname + + # Ignore sorting rules on imports inside blocks. + if node.col_offset == 0: + if lastname and name < lastname: + yield 'imports not lexically sorted: %s < %s' % ( + name, lastname) + + lastname = name + + # stdlib imports should be before local imports. + stdlib = name in stdlib_modules + if stdlib and seenlocal and node.col_offset == 0: + yield 'stdlib import follows local import: %s' % name + + if not stdlib: + seenlocal = True + + # Import of sibling modules should use relative imports. + topname = name.split('.')[0] + if topname == topmodule: + yield 'import should be relative: %s' % name + + if name in requirealias and asname != requirealias[name]: + yield '%s module must be "as" aliased to %s' % ( + name, requirealias[name]) + + elif isinstance(node, ast.ImportFrom): + # Resolve the full imported module name. + if node.level > 0: + fullname = '.'.join(module.split('.')[:-node.level]) + if node.module: + fullname += '.%s' % node.module + else: + assert node.module + fullname = node.module + + topname = fullname.split('.')[0] + if topname == topmodule: + yield 'import should be relative: %s' % fullname + + # __future__ is special since it needs to come first and use + # symbol import. + if fullname != '__future__': + if not fullname or fullname in stdlib_modules: + yield 'relative import of stdlib module' + else: + seenlocal = True + + # Direct symbol import is only allowed from certain modules and + # must occur before non-symbol imports. + if node.module and node.col_offset == 0: + if fullname not in allowsymbolimports: + yield 'direct symbol import from %s' % fullname + + if seennonsymbolrelative: + yield ('symbol import follows non-symbol import: %s' % + fullname) + + if not node.module: + assert node.level + seennonsymbolrelative = True + + # Only allow 1 group per level. + if node.level in seenlevels and node.col_offset == 0: + yield 'multiple "from %s import" statements' % ( + '.' * node.level) + + # Higher-level groups come before lower-level groups. + if any(node.level > l for l in seenlevels): + yield 'higher-level import should come first: %s' % ( + fullname) + + seenlevels.add(node.level) + + # Entries in "from .X import ( ... )" lists must be lexically + # sorted. + lastentryname = None + + for n in node.names: + if lastentryname and n.name < lastentryname: + yield 'imports from %s not lexically sorted: %s < %s' % ( + fullname, n.name, lastentryname) + + lastentryname = n.name + + if n.name in requirealias and n.asname != requirealias[n.name]: + yield '%s from %s must be "as" aliased to %s' % ( + n.name, fullname, requirealias[n.name]) def verify_stdlib_on_own_line(root): """Given some python source, verify that stdlib imports are done