Do not reparent stuff listed in __all__ of a public module.

Give priority to public modules in the re-export sorting.

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]

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

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.