This is an archive of the discontinued Mercurial Phabricator instance.

Differential D551

import-checker: allow direct symbol import if the symbol is a module
AbandonedPublic

Authored by quark on Aug 28 2017, 9:06 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Summary

This would make the checker more friendly for 3rd-party code. For example,

In remotefilelog/x.py, it may have:

from . import shallowutils

This triggers "direct symbol import shallowutils from remotefilelog" today.
Since "shallowutils" itself is a module, the import should be allowed. This
patch makes it so.

It seems the warning is mainly to avoid the situation where other code could
wrap some symbols (typically by extensions.wrapfunction) after import, and
the existing code would still be using old symbols. But it's rare to have
an entire module replaced. So I think this change is reasonable.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Aug 28 2017, 9:06 PM
yuja requested changes to this revision.Aug 31 2017, 10:30 AM
yuja added a subscriber: yuja.

In remotefilelog/x.py, it may have:

from . import shallowutils

This triggers "direct symbol import shallowutils from remotefilelog" today.

Perhaps you didn't pass remotefilelog/__init__.py and remotefilelog/shallowutils.py
to import-checker.py. If they are counted as local modules, the error should disappear.

tests/test-imports-checker.t
134

This has to be updated to actually test the "direct symbol import".

This revision now requires changes to proceed.Aug 31 2017, 10:30 AM
quark abandoned this revision.Aug 31 2017, 5:36 PM

You're right. I checked the output again and it worked as expected. I think it was caused by me experimenting with stuff and modified localmods with .__init__ removed (which never got sent). I'll drop this patch. The next patch is still relevant.

Revision Contents
Changeset List

Diff 1375

contrib/import-checker.py

# symbol import. # symbol import.
if fullname != '__future__': if fullname != '__future__':
if not fullname or fullname in stdlib_modules: if not fullname or fullname in stdlib_modules:
yield msg('relative import of stdlib module') yield msg('relative import of stdlib module')
else: else:
seenlocal = fullname seenlocal = fullname
# Direct symbol import is only allowed from certain modules and # Direct symbol import is only allowed from certain modules and
# must occur before non-symbol imports. # must occur before non-symbol imports, or the symbol itself is a
# local module.
found = fromlocal(node.module, node.level) found = fromlocal(node.module, node.level)
if found and found[2]: # node.module is a package if found and found[2]: # node.module is a package
prefix = found[0] + '.' prefix = found[0] + '.'
symbols = (n.name for n in node.names symbols = (n.name for n in node.names
if not fromlocal(prefix + n.name)) if not fromlocal(prefix + n.name))
else: else:
symbols = (n.name for n in node.names) symbols = (n.name for n in node.names)
symbols = [sym for sym in symbols if sym not in directsymbols] symbols = [sym for sym in symbols if sym not in directsymbols]
if node.module and node.col_offset == root_col_offset: if node.module and node.col_offset == root_col_offset:
if symbols and fullname not in allowsymbolimports: if (symbols and fullname not in allowsymbolimports
and fullname not in localmods
and fullname + '.__init__' not in localmods):
yield msg('direct symbol import %s from %s', yield msg('direct symbol import %s from %s',
', '.join(symbols), fullname) ', '.join(symbols), fullname)
if symbols and seennonsymbollocal: if symbols and seennonsymbollocal:
yield msg('symbol import follows non-symbol import: %s', yield msg('symbol import follows non-symbol import: %s',
fullname) fullname)
if not symbols and fullname not in stdlib_modules: if not symbols and fullname not in stdlib_modules:
seennonsymbollocal = True seennonsymbollocal = True

tests/test-imports-checker.t

> from testpackage import unsorted > from testpackage import unsorted
> from mercurial.node import hex > from mercurial.node import hex
> EOF > EOF
$ $PYTHON "$import_checker" testpackage*/*.py testpackage/subpackage/*.py $ $PYTHON "$import_checker" testpackage*/*.py testpackage/subpackage/*.py
testpackage/importalias.py:2: ui module must be "as" aliased to uimod testpackage/importalias.py:2: ui module must be "as" aliased to uimod
testpackage/importfromalias.py:2: ui from testpackage must be "as" aliased to uimod testpackage/importfromalias.py:2: ui from testpackage must be "as" aliased to uimod
testpackage/importfromrelative.py:2: import should be relative: testpackage.unsorted testpackage/importfromrelative.py:2: import should be relative: testpackage.unsorted
testpackage/importfromrelative.py:2: direct symbol import foo from testpackage.unsorted
testpackage/importsymbolfromsub.py:2: direct symbol import nonmodule from testpackage.subpackage
testpackage/latesymbolimport.py:3: symbol import follows non-symbol import: mercurial.node testpackage/latesymbolimport.py:3: symbol import follows non-symbol import: mercurial.node
yujaUnsubmitted
Not Done

This has to be updated to actually test the "direct symbol import".

yuja: This has to be updated to actually test the "direct symbol import".
testpackage/multiple.py:2: multiple imported names: os, sys testpackage/multiple.py:2: multiple imported names: os, sys
testpackage/multiplegroups.py:3: multiple "from . import" statements testpackage/multiplegroups.py:3: multiple "from . import" statements
testpackage/relativestdlib.py:2: relative import of stdlib module testpackage/relativestdlib.py:2: relative import of stdlib module
testpackage/requirerelative.py:2: import should be relative: testpackage.unsorted testpackage/requirerelative.py:2: import should be relative: testpackage.unsorted
testpackage/sortedentries.py:2: imports from testpackage not lexically sorted: bar < foo testpackage/sortedentries.py:2: imports from testpackage not lexically sorted: bar < foo
testpackage/stdafterlocal.py:3: stdlib import "os" follows local import: testpackage testpackage/stdafterlocal.py:3: stdlib import "os" follows local import: testpackage
testpackage/subpackage/levelpriority.py:3: higher-level import should come first: testpackage testpackage/subpackage/levelpriority.py:3: higher-level import should come first: testpackage
testpackage/subpackage/localimport.py:7: multiple "from .. import" statements testpackage/subpackage/localimport.py:7: multiple "from .. import" statements
testpackage/subpackage/localimport.py:8: import should be relative: testpackage.subpackage.levelpriority testpackage/subpackage/localimport.py:8: import should be relative: testpackage.subpackage.levelpriority
testpackage/symbolimport.py:2: direct symbol import foo from testpackage.unsorted
testpackage/unsorted.py:3: imports not lexically sorted: os < sys testpackage/unsorted.py:3: imports not lexically sorted: os < sys
testpackage2/latesymbolimport.py:3: symbol import follows non-symbol import: mercurial.node testpackage2/latesymbolimport.py:3: symbol import follows non-symbol import: mercurial.node
[1] [1]