Show More
@@ -8,6 +8,33 b' import sys' | |||||
8 | import BaseHTTPServer |
|
8 | import BaseHTTPServer | |
9 | import zlib |
|
9 | import zlib | |
10 |
|
10 | |||
|
11 | # Whitelist of modules that symbols can be directly imported from. | |||
|
12 | allowsymbolimports = ( | |||
|
13 | '__future__', | |||
|
14 | 'mercurial.i18n', | |||
|
15 | 'mercurial.node', | |||
|
16 | ) | |||
|
17 | ||||
|
18 | # Modules that must be aliased because they are commonly confused with | |||
|
19 | # common variables and can create aliasing and readability issues. | |||
|
20 | requirealias = { | |||
|
21 | 'ui': 'uimod', | |||
|
22 | } | |||
|
23 | ||||
|
24 | def usingabsolute(root): | |||
|
25 | """Whether absolute imports are being used.""" | |||
|
26 | if sys.version_info[0] >= 3: | |||
|
27 | return True | |||
|
28 | ||||
|
29 | for node in ast.walk(root): | |||
|
30 | if isinstance(node, ast.ImportFrom): | |||
|
31 | if node.module == '__future__': | |||
|
32 | for n in node.names: | |||
|
33 | if n.name == 'absolute_import': | |||
|
34 | return True | |||
|
35 | ||||
|
36 | return False | |||
|
37 | ||||
11 | def dotted_name_of_path(path, trimpure=False): |
|
38 | def dotted_name_of_path(path, trimpure=False): | |
12 | """Given a relative path to a source file, return its dotted module name. |
|
39 | """Given a relative path to a source file, return its dotted module name. | |
13 |
|
40 | |||
@@ -273,11 +300,158 b' def imported_modules(source, modulename,' | |||||
273 | yield found[1] |
|
300 | yield found[1] | |
274 |
|
301 | |||
275 | def verify_import_convention(module, source): |
|
302 | def verify_import_convention(module, source): | |
276 |
"""Verify imports match our established coding convention. |
|
303 | """Verify imports match our established coding convention. | |
|
304 | ||||
|
305 | We have 2 conventions: legacy and modern. The modern convention is in | |||
|
306 | effect when using absolute imports. | |||
|
307 | ||||
|
308 | The legacy convention only looks for mixed imports. The modern convention | |||
|
309 | is much more thorough. | |||
|
310 | """ | |||
277 | root = ast.parse(source) |
|
311 | root = ast.parse(source) | |
|
312 | absolute = usingabsolute(root) | |||
278 |
|
313 | |||
|
314 | if absolute: | |||
|
315 | return verify_modern_convention(module, root) | |||
|
316 | else: | |||
279 | return verify_stdlib_on_own_line(root) |
|
317 | return verify_stdlib_on_own_line(root) | |
280 |
|
318 | |||
|
319 | def verify_modern_convention(module, root): | |||
|
320 | """Verify a file conforms to the modern import convention rules. | |||
|
321 | ||||
|
322 | The rules of the modern convention are: | |||
|
323 | ||||
|
324 | * Ordering is stdlib followed by local imports. Each group is lexically | |||
|
325 | sorted. | |||
|
326 | * Importing multiple modules via "import X, Y" is not allowed: use | |||
|
327 | separate import statements. | |||
|
328 | * Importing multiple modules via "from X import ..." is allowed if using | |||
|
329 | parenthesis and one entry per line. | |||
|
330 | * Only 1 relative import statement per import level ("from .", "from ..") | |||
|
331 | is allowed. | |||
|
332 | * Relative imports from higher levels must occur before lower levels. e.g. | |||
|
333 | "from .." must be before "from .". | |||
|
334 | * Imports from peer packages should use relative import (e.g. do not | |||
|
335 | "import mercurial.foo" from a "mercurial.*" module). | |||
|
336 | * Symbols can only be imported from specific modules (see | |||
|
337 | `allowsymbolimports`). For other modules, first import the module then | |||
|
338 | assign the symbol to a module-level variable. In addition, these imports | |||
|
339 | must be performed before other relative imports. This rule only | |||
|
340 | applies to import statements outside of any blocks. | |||
|
341 | * Relative imports from the standard library are not allowed. | |||
|
342 | * Certain modules must be aliased to alternate names to avoid aliasing | |||
|
343 | and readability problems. See `requirealias`. | |||
|
344 | """ | |||
|
345 | topmodule = module.split('.')[0] | |||
|
346 | ||||
|
347 | # Whether a local/non-stdlib import has been performed. | |||
|
348 | seenlocal = False | |||
|
349 | # Whether a relative, non-symbol import has been seen. | |||
|
350 | seennonsymbolrelative = False | |||
|
351 | # The last name to be imported (for sorting). | |||
|
352 | lastname = None | |||
|
353 | # Relative import levels encountered so far. | |||
|
354 | seenlevels = set() | |||
|
355 | ||||
|
356 | for node in ast.walk(root): | |||
|
357 | if isinstance(node, ast.Import): | |||
|
358 | # Disallow "import foo, bar" and require separate imports | |||
|
359 | # for each module. | |||
|
360 | if len(node.names) > 1: | |||
|
361 | yield 'multiple imported names: %s' % ', '.join( | |||
|
362 | n.name for n in node.names) | |||
|
363 | ||||
|
364 | name = node.names[0].name | |||
|
365 | asname = node.names[0].asname | |||
|
366 | ||||
|
367 | # Ignore sorting rules on imports inside blocks. | |||
|
368 | if node.col_offset == 0: | |||
|
369 | if lastname and name < lastname: | |||
|
370 | yield 'imports not lexically sorted: %s < %s' % ( | |||
|
371 | name, lastname) | |||
|
372 | ||||
|
373 | lastname = name | |||
|
374 | ||||
|
375 | # stdlib imports should be before local imports. | |||
|
376 | stdlib = name in stdlib_modules | |||
|
377 | if stdlib and seenlocal and node.col_offset == 0: | |||
|
378 | yield 'stdlib import follows local import: %s' % name | |||
|
379 | ||||
|
380 | if not stdlib: | |||
|
381 | seenlocal = True | |||
|
382 | ||||
|
383 | # Import of sibling modules should use relative imports. | |||
|
384 | topname = name.split('.')[0] | |||
|
385 | if topname == topmodule: | |||
|
386 | yield 'import should be relative: %s' % name | |||
|
387 | ||||
|
388 | if name in requirealias and asname != requirealias[name]: | |||
|
389 | yield '%s module must be "as" aliased to %s' % ( | |||
|
390 | name, requirealias[name]) | |||
|
391 | ||||
|
392 | elif isinstance(node, ast.ImportFrom): | |||
|
393 | # Resolve the full imported module name. | |||
|
394 | if node.level > 0: | |||
|
395 | fullname = '.'.join(module.split('.')[:-node.level]) | |||
|
396 | if node.module: | |||
|
397 | fullname += '.%s' % node.module | |||
|
398 | else: | |||
|
399 | assert node.module | |||
|
400 | fullname = node.module | |||
|
401 | ||||
|
402 | topname = fullname.split('.')[0] | |||
|
403 | if topname == topmodule: | |||
|
404 | yield 'import should be relative: %s' % fullname | |||
|
405 | ||||
|
406 | # __future__ is special since it needs to come first and use | |||
|
407 | # symbol import. | |||
|
408 | if fullname != '__future__': | |||
|
409 | if not fullname or fullname in stdlib_modules: | |||
|
410 | yield 'relative import of stdlib module' | |||
|
411 | else: | |||
|
412 | seenlocal = True | |||
|
413 | ||||
|
414 | # Direct symbol import is only allowed from certain modules and | |||
|
415 | # must occur before non-symbol imports. | |||
|
416 | if node.module and node.col_offset == 0: | |||
|
417 | if fullname not in allowsymbolimports: | |||
|
418 | yield 'direct symbol import from %s' % fullname | |||
|
419 | ||||
|
420 | if seennonsymbolrelative: | |||
|
421 | yield ('symbol import follows non-symbol import: %s' % | |||
|
422 | fullname) | |||
|
423 | ||||
|
424 | if not node.module: | |||
|
425 | assert node.level | |||
|
426 | seennonsymbolrelative = True | |||
|
427 | ||||
|
428 | # Only allow 1 group per level. | |||
|
429 | if node.level in seenlevels and node.col_offset == 0: | |||
|
430 | yield 'multiple "from %s import" statements' % ( | |||
|
431 | '.' * node.level) | |||
|
432 | ||||
|
433 | # Higher-level groups come before lower-level groups. | |||
|
434 | if any(node.level > l for l in seenlevels): | |||
|
435 | yield 'higher-level import should come first: %s' % ( | |||
|
436 | fullname) | |||
|
437 | ||||
|
438 | seenlevels.add(node.level) | |||
|
439 | ||||
|
440 | # Entries in "from .X import ( ... )" lists must be lexically | |||
|
441 | # sorted. | |||
|
442 | lastentryname = None | |||
|
443 | ||||
|
444 | for n in node.names: | |||
|
445 | if lastentryname and n.name < lastentryname: | |||
|
446 | yield 'imports from %s not lexically sorted: %s < %s' % ( | |||
|
447 | fullname, n.name, lastentryname) | |||
|
448 | ||||
|
449 | lastentryname = n.name | |||
|
450 | ||||
|
451 | if n.name in requirealias and n.asname != requirealias[n.name]: | |||
|
452 | yield '%s from %s must be "as" aliased to %s' % ( | |||
|
453 | n.name, fullname, requirealias[n.name]) | |||
|
454 | ||||
281 | def verify_stdlib_on_own_line(root): |
|
455 | def verify_stdlib_on_own_line(root): | |
282 | """Given some python source, verify that stdlib imports are done |
|
456 | """Given some python source, verify that stdlib imports are done | |
283 | in separate statements from relative local module imports. |
|
457 | in separate statements from relative local module imports. |
@@ -8,6 +8,100 b" it's working correctly." | |||||
8 | $ export TERM |
|
8 | $ export TERM | |
9 | $ python -m doctest $import_checker |
|
9 | $ python -m doctest $import_checker | |
10 |
|
10 | |||
|
11 | Run additional tests for the import checker | |||
|
12 | ||||
|
13 | $ mkdir testpackage | |||
|
14 | ||||
|
15 | $ cat > testpackage/multiple.py << EOF | |||
|
16 | > from __future__ import absolute_import | |||
|
17 | > import os, sys | |||
|
18 | > EOF | |||
|
19 | ||||
|
20 | $ cat > testpackage/unsorted.py << EOF | |||
|
21 | > from __future__ import absolute_import | |||
|
22 | > import sys | |||
|
23 | > import os | |||
|
24 | > EOF | |||
|
25 | ||||
|
26 | $ cat > testpackage/stdafterlocal.py << EOF | |||
|
27 | > from __future__ import absolute_import | |||
|
28 | > from . import unsorted | |||
|
29 | > import os | |||
|
30 | > EOF | |||
|
31 | ||||
|
32 | $ cat > testpackage/requirerelative.py << EOF | |||
|
33 | > from __future__ import absolute_import | |||
|
34 | > import testpackage.unsorted | |||
|
35 | > EOF | |||
|
36 | ||||
|
37 | $ cat > testpackage/importalias.py << EOF | |||
|
38 | > from __future__ import absolute_import | |||
|
39 | > import ui | |||
|
40 | > EOF | |||
|
41 | ||||
|
42 | $ cat > testpackage/relativestdlib.py << EOF | |||
|
43 | > from __future__ import absolute_import | |||
|
44 | > from .. import os | |||
|
45 | > EOF | |||
|
46 | ||||
|
47 | $ cat > testpackage/symbolimport.py << EOF | |||
|
48 | > from __future__ import absolute_import | |||
|
49 | > from .unsorted import foo | |||
|
50 | > EOF | |||
|
51 | ||||
|
52 | $ cat > testpackage/latesymbolimport.py << EOF | |||
|
53 | > from __future__ import absolute_import | |||
|
54 | > from . import unsorted | |||
|
55 | > from mercurial.node import hex | |||
|
56 | > EOF | |||
|
57 | ||||
|
58 | $ cat > testpackage/multiplegroups.py << EOF | |||
|
59 | > from __future__ import absolute_import | |||
|
60 | > from . import unsorted | |||
|
61 | > from . import more | |||
|
62 | > EOF | |||
|
63 | ||||
|
64 | $ mkdir testpackage/subpackage | |||
|
65 | $ cat > testpackage/subpackage/levelpriority.py << EOF | |||
|
66 | > from __future__ import absolute_import | |||
|
67 | > from . import foo | |||
|
68 | > from .. import parent | |||
|
69 | > EOF | |||
|
70 | ||||
|
71 | $ cat > testpackage/sortedentries.py << EOF | |||
|
72 | > from __future__ import absolute_import | |||
|
73 | > from . import ( | |||
|
74 | > foo, | |||
|
75 | > bar, | |||
|
76 | > ) | |||
|
77 | > EOF | |||
|
78 | ||||
|
79 | $ cat > testpackage/importfromalias.py << EOF | |||
|
80 | > from __future__ import absolute_import | |||
|
81 | > from . import ui | |||
|
82 | > EOF | |||
|
83 | ||||
|
84 | $ cat > testpackage/importfromrelative.py << EOF | |||
|
85 | > from __future__ import absolute_import | |||
|
86 | > from testpackage.unsorted import foo | |||
|
87 | > EOF | |||
|
88 | ||||
|
89 | $ python "$import_checker" testpackage/*.py testpackage/subpackage/*.py | |||
|
90 | testpackage/importalias.py ui module must be "as" aliased to uimod | |||
|
91 | testpackage/importfromalias.py ui from testpackage must be "as" aliased to uimod | |||
|
92 | testpackage/importfromrelative.py import should be relative: testpackage.unsorted | |||
|
93 | testpackage/importfromrelative.py direct symbol import from testpackage.unsorted | |||
|
94 | testpackage/latesymbolimport.py symbol import follows non-symbol import: mercurial.node | |||
|
95 | testpackage/multiple.py multiple imported names: os, sys | |||
|
96 | testpackage/multiplegroups.py multiple "from . import" statements | |||
|
97 | testpackage/relativestdlib.py relative import of stdlib module | |||
|
98 | testpackage/requirerelative.py import should be relative: testpackage.unsorted | |||
|
99 | testpackage/sortedentries.py imports from testpackage not lexically sorted: bar < foo | |||
|
100 | testpackage/stdafterlocal.py stdlib import follows local import: os | |||
|
101 | testpackage/subpackage/levelpriority.py higher-level import should come first: testpackage | |||
|
102 | testpackage/symbolimport.py direct symbol import from testpackage.unsorted | |||
|
103 | testpackage/unsorted.py imports not lexically sorted: os < sys | |||
|
104 | ||||
11 | $ cd "$TESTDIR"/.. |
|
105 | $ cd "$TESTDIR"/.. | |
12 |
|
106 | |||
13 | There are a handful of cases here that require renaming a module so it |
|
107 | There are a handful of cases here that require renaming a module so it |
General Comments 0
You need to be logged in to leave comments.
Login now