From d5d176a0a37dee3349761b36f9b1d27cffc44123 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Sat, 30 Sep 2023 19:34:35 -0400 Subject: [PATCH] Do not reparent stuff listed in __all__ of a public module. Give priority to public modules in the re-export sorting. --- pydoctor/astbuilder.py | 65 +++++++++++++++++++++----------- pydoctor/test/test_astbuilder.py | 45 +++++++++++++++++----- pydoctor/test/test_packages.py | 7 ++-- 3 files changed, 81 insertions(+), 36 deletions(-) diff --git a/pydoctor/astbuilder.py b/pydoctor/astbuilder.py index e3038e381..55320fd3d 100644 --- a/pydoctor/astbuilder.py +++ b/pydoctor/astbuilder.py @@ -2,6 +2,7 @@ import ast from collections import defaultdict +from statistics import mean import sys from functools import partial @@ -186,32 +187,32 @@ def _handleReExport(info:'ReExport', elsewhere:Collection['ReExport']) -> None: as_name = info.as_name target_parent = target.parent assert isinstance(target_parent, model.Module) + + # Remember that this name is re-exported + target_parent.elsewhere_contents[target.name] = target + + extra_msg = '' + + for e in elsewhere: + e.new_parent.elsewhere_contents[e.as_name] = target + if not extra_msg: + extra_msg += ', also available at ' + extra_msg += f"'{e.new_parent.fullName()}.{e.as_name}'" + else: + extra_msg += f" and '{e.new_parent.fullName()}.{e.as_name}'" + if as_name != target.name: new_parent.system.msg( "astbuilder", - f"moving {target.fullName()!r} into {new_parent.fullName()!r} as {as_name!r}") + f"moving {target.fullName()!r} into {new_parent.fullName()!r} as {as_name!r}{extra_msg}") else: new_parent.system.msg( "astbuilder", - f"moving {target.fullName()!r} into {new_parent.fullName()!r}") - - # Remember that this name is re-exported - target_parent.elsewhere_contents[target.name] = target - for e in elsewhere: - new_parent.system.msg( - "astbuilder", - f"also available at '{e.new_parent.fullName()}.{e.as_name}'") - e.new_parent.elsewhere_contents[e.as_name] = target - + f"moving {target.fullName()!r} into {new_parent.fullName()!r}{extra_msg}") + target.reparent(new_parent, as_name) - # if origin_module.all is None or origin_name not in origin_module.all: - # else: - # new_parent.system.msg( - # "astbuilder", - # f"not moving {target.fullName()} into {new_parent.fullName()}, " - # f"because {origin_name!r} is already exported in {modname}.__all__") def getModuleExports(mod:'model.Module') -> Collection[str]: # Fetch names to export. @@ -242,6 +243,11 @@ class ReExport: origin_module: model.Module target: model.Documentable +def _exports_order(r:ReExport) -> object: + return (-r.new_parent.privacyClass.value, + r.new_parent.fullName().count('.'), + -len(r.as_name)) + def _maybeExistingNameOverridesImport(mod:model.Module, local_name:str, imp:model.Import, target:model.Documentable) -> bool: @@ -308,15 +314,28 @@ def processReExports(system:'model.System') -> None: for target, _exports in exports_per_target.items(): elsewhere = [] + + if isinstance(target.parent, model.Module) and target.parent.all is not None \ + and target.name in target.parent.all \ + and target.parent.privacyClass is model.PrivacyClass.PUBLIC: + + target.system.msg( + "astbuilder", + f"not moving {target.fullName()} into {' or '.join(repr(e.new_parent.fullName()) for e in _exports)}, " + f"because {target.name!r} is already exported in public module {target.parent.fullName()!r}") + + for e in _exports: + e.new_parent.elsewhere_contents[e.as_name] = target + + continue + assert len(_exports) > 0 if len(_exports) > 1: - # when an object has several re-exports, the module with the lowest number + # when an object has several re-exports, the public module with the lowest number # of dot in it's name is choosen, if there is an equality, the longer local name - # if choosen - # TODO: move this into a system method - # TODO: do not move objects inside a private module - # TODO: do not move objects when they are listed in __all__ of a public module - _exports.sort(key=lambda r:(r.new_parent.fullName().count('.'), -len(r.as_name))) + # is choosen + + _exports.sort(key=_exports_order) elsewhere.extend(_exports[1:]) reexport = _exports[0] diff --git a/pydoctor/test/test_astbuilder.py b/pydoctor/test/test_astbuilder.py index 5668e986e..fad7fa761 100644 --- a/pydoctor/test/test_astbuilder.py +++ b/pydoctor/test/test_astbuilder.py @@ -2873,21 +2873,48 @@ class Slc:... def test_reparenting_from_module_that_defines__all__(systemcls: Type[model.System], capsys:CapSys) -> None: """ Even if a module defined it's own __all__ attribute, - we can reparent it's direct children to a new module, - but only if it's a private module. + we can reparent it's direct children to a new module + but only when the origin module has a lower privacy class + (i.e reparenting from a private module to a plublic module), otherwise the name stays there. """ - src = '''\ + _src = '''\ class cls:... - __all__ = ['cls'] + class cls3:... + class cls4:... + __all__ = ['cls', 'cls3', 'cls4'] + ''' + src = ''' + class cls2:... + __all__ = ['cls2'] + ''' + pack = '''\ + from ._src import cls + from .src import cls2 + __all__=["cls","cls2"] + ''' + subpack = '''\ + from .._src import cls3 + __all__=["cls3"] + ''' + private = '''\ + from pack._src import cls3, cls4 + __all__ = ['cls3', 'cls4'] ''' - pack = 'from ._src import cls; __all__=["cls"]' system = systemcls() builder = system.systemBuilder(system) + builder.addModuleString(private, '_private') builder.addModuleString(pack, 'pack', is_package=True) - builder.addModuleString(src, '_src', parent_name='pack') + builder.addModuleString(subpack, 'subpack', parent_name='pack', is_package=True) + builder.addModuleString(_src, '_src', parent_name='pack') + builder.addModuleString(src, 'src', parent_name='pack') builder.buildModules() - assert capsys.readouterr().out == "moving 'pack._src.cls' into 'pack'\n" + assert capsys.readouterr().out == ( + "moving 'pack._src.cls3' into 'pack.subpack', also available at '_private.cls3'\n" + "moving 'pack._src.cls4' into '_private'\n" + "moving 'pack._src.cls' into 'pack'\n" + "not moving pack.src.cls2 into 'pack', because 'cls2' is already exported in public module 'pack.src'\n") + assert system.allobjects['pack.cls'] is system.allobjects['pack._src'].elsewhere_contents['cls'] # type:ignore @systemcls_param @@ -2949,7 +2976,7 @@ class Cls:... builder.addModuleString(src, 'src', parent_name='pack.subpack') builder.buildModules() - assert capsys.readouterr().out == ("moving 'pack.subpack.src.Cls' into 'pack'\n" + assert capsys.readouterr().out == ("moving 'pack.subpack.src.Cls' into 'pack', " "also available at 'pack.subpack.Cls'\n") assert system.allobjects['pack.Cls'] is system.allobjects['pack.subpack'].elsewhere_contents['Cls'] # type:ignore @@ -2977,7 +3004,7 @@ class DistinguishedName:... builder.addModuleString(src, 'src', parent_name='pack.subpack') builder.buildModules() - assert capsys.readouterr().out == ("moving 'pack.subpack.src.DistinguishedName' into 'pack' as 'DisName'\n" + assert capsys.readouterr().out == ("moving 'pack.subpack.src.DistinguishedName' into 'pack' as 'DisName', " "also available at 'pack.DN'\n") assert system.allobjects['pack.DisName'] is system.allobjects['pack'].elsewhere_contents['DN'] # type:ignore diff --git a/pydoctor/test/test_packages.py b/pydoctor/test/test_packages.py index 8223b4943..37034c655 100644 --- a/pydoctor/test/test_packages.py +++ b/pydoctor/test/test_packages.py @@ -57,10 +57,9 @@ def test_allgames() -> None: assert isinstance(mod1, model.Module) mod2 = system.allobjects['allgames.mod2'] assert isinstance(mod2, model.Module) - # changed in 2023 - # InSourceAll is moved into mod2 as well as NotInSourceAll. - assert 'InSourceAll' not in mod1.contents - assert 'InSourceAll' in mod2.contents + # InSourceAll is not moved into mod2 because it's defined in __all__ and module is public. + assert 'InSourceAll' in mod1.contents + assert 'InSourceAll' not in mod2.contents assert 'NotInSourceAll' in mod2.contents # Source paths must be unaffected by the move, so that error messages # point to the right source code.