From 7cad3712bcde51254fc0f498fd55a99087709325 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Tue, 2 Dec 2025 17:23:32 +0000 Subject: [PATCH 01/12] Fix bug with FakeFileSystem Prior to this, subsequent sibling packages in the contents string would be incorrectly nested under the directory the line above, due to an assumption that the contents string would be dedented. This takes account of that. --- rust/src/filesystem.rs | 10 ++++------ tests/unit/adaptors/test_filesystem.py | 3 +++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/rust/src/filesystem.rs b/rust/src/filesystem.rs index c234aa64..8f3a74f0 100644 --- a/rust/src/filesystem.rs +++ b/rust/src/filesystem.rs @@ -323,7 +323,7 @@ fn parse_indented_file_system_string(file_system_string: &str) -> HashMap = HashMap::new(); let mut path_stack: Vec = Vec::new(); // Stores current directory path components let mut first_line = true; // Flag to handle the very first path component - + let mut first_line_indent: usize = 0; // Normalize newlines and split into lines let buffer = file_system_string.replace("\r\n", "\n"); let lines: Vec<&str> = buffer.split('\n').collect(); @@ -334,27 +334,25 @@ fn parse_indented_file_system_string(file_system_string: &str) -> HashMap current_depth { path_stack.pop(); } - // If the current line is a file, append it to the path for inserting into map, // then pop it off so that subsequent siblings are correctly handled. // If it's a directory, append it and it stays on the stack for its children. diff --git a/tests/unit/adaptors/test_filesystem.py b/tests/unit/adaptors/test_filesystem.py index e769902b..1c526bc2 100644 --- a/tests/unit/adaptors/test_filesystem.py +++ b/tests/unit/adaptors/test_filesystem.py @@ -40,6 +40,7 @@ def test_split(self, path, expected): ("/path/to/mypackage/readme.txt", True), ("/path/to/mypackage/foo/one.txt", True), ("/path/to/mypackage/foo/two/green.txt", True), + ("/path/to/mypackage/bar/blue.txt", True), ("/path/to/nonexistent.txt", False), ("/path/to/mypackage/purple.txt", False), ], @@ -53,6 +54,8 @@ def test_exists_content_only(self, file_name, expected): one.txt two/ green.txt + bar/ + blue.txt """ ) From 474827e5c1f25b3ac68e5f1a9d28ce978ee899ae Mon Sep 17 00:00:00 2001 From: David Seddon Date: Tue, 2 Dec 2025 17:28:38 +0000 Subject: [PATCH 02/12] Add failing test --- tests/unit/application/test_usecases.py | 45 +++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/unit/application/test_usecases.py b/tests/unit/application/test_usecases.py index 5f7adbe8..a6be720a 100644 --- a/tests/unit/application/test_usecases.py +++ b/tests/unit/application/test_usecases.py @@ -66,6 +66,51 @@ class FakePackageFinder(BaseFakePackageFinder): with pytest.raises(ValueError, match="Cannot find children of a squashed module."): graph.find_children(module) + @pytest.mark.xfail(reason="Not yet supported") + def test_namespace_package_passed_as_root(self): + file_system = FakeFileSystem( + contents=""" + /path/to/mypackage/ + foo/ + __init__.py + one.py + two/ + __init__.py + green.py + blue.py + bar/ + __init__.py + three.py + /different-path/to/mypackage/ + foobar/ + __init__.py + four.py + """, + ) + + class FakePackageFinder(BaseFakePackageFinder): + directory_map = { + # TODO - PackageFinder assumes only a single directory per package. + # This isn't the case for namespace packages. + "mypackage": "/path/to/mypackage" + } + + with override_settings(FILE_SYSTEM=file_system, PACKAGE_FINDER=FakePackageFinder()): + graph = usecases.build_graph("mypackage") + + assert graph.modules == { + "mypackage", + "mypackage.foo", + "mypackage.foo.one", + "mypackage.foo.two", + "mypackage.foo.two.green", + "mypackage.foo.two.blue", + "mypackage.bar", + "mypackage.bar.three", + "mypackage.foobar", + "mypackage.foobar.four", + } + def test_boolean_additional_package_raises_type_error(self): """ Tests that a useful error message if build_graph is called From 75fd1751849a514523466002bed3c8ad6f6834da Mon Sep 17 00:00:00 2001 From: David Seddon Date: Tue, 2 Dec 2025 17:56:12 +0000 Subject: [PATCH 03/12] Make package finder return multiple directories This doesn't change the implementation yet, but it prepares us for supporting namespace packages passed in. --- src/grimp/adaptors/packagefinder.py | 7 ++++++- src/grimp/application/ports/packagefinder.py | 4 ++-- src/grimp/application/usecases.py | 10 +++++++++- tests/adaptors/packagefinder.py | 6 +++--- tests/unit/adaptors/test_packagefinder.py | 18 +++++++++--------- 5 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/grimp/adaptors/packagefinder.py b/src/grimp/adaptors/packagefinder.py index e87f644e..4115b60f 100644 --- a/src/grimp/adaptors/packagefinder.py +++ b/src/grimp/adaptors/packagefinder.py @@ -12,7 +12,7 @@ class ImportLibPackageFinder(AbstractPackageFinder): - def determine_package_directory( + def _determine_package_directory( self, package_name: str, file_system: AbstractFileSystem ) -> str: # TODO - do we need to add the current working directory here? @@ -34,6 +34,11 @@ def determine_package_directory( "adding an __init__.py file should fix the problem." ) + def determine_package_directories( + self, package_name: str, file_system: AbstractFileSystem + ) -> set[str]: + return {self._determine_package_directory(package_name, file_system)} + def _is_a_package(self, spec: ModuleSpec, file_system: AbstractFileSystem) -> bool: assert spec.origin filename = file_system.split(spec.origin)[1] diff --git a/src/grimp/application/ports/packagefinder.py b/src/grimp/application/ports/packagefinder.py index 09c4da45..30b6d0ed 100644 --- a/src/grimp/application/ports/packagefinder.py +++ b/src/grimp/application/ports/packagefinder.py @@ -5,7 +5,7 @@ class AbstractPackageFinder(abc.ABC): @abc.abstractmethod - def determine_package_directory( + def determine_package_directories( self, package_name: str, file_system: AbstractFileSystem - ) -> str: + ) -> set[str]: raise NotImplementedError diff --git a/src/grimp/application/usecases.py b/src/grimp/application/usecases.py index e7503d29..7f544863 100644 --- a/src/grimp/application/usecases.py +++ b/src/grimp/application/usecases.py @@ -3,6 +3,7 @@ """ from typing import cast + from collections.abc import Sequence, Iterable from .scanning import scan_imports @@ -12,6 +13,7 @@ from ..application.ports.modulefinder import AbstractModuleFinder, FoundPackage, ModuleFile from ..application.ports.packagefinder import AbstractPackageFinder from ..domain.valueobjects import DirectImport, Module +from .. import exceptions from .config import settings @@ -79,9 +81,15 @@ def _find_packages( found_packages: set[FoundPackage] = set() for package_name in package_names: - package_directory = package_finder.determine_package_directory( + package_directories = package_finder.determine_package_directories( package_name=package_name, file_system=file_system ) + try: + [package_directory] = package_directories + except ValueError: + # TODO handle multiple directories. + raise exceptions.NamespacePackageEncountered + found_package = module_finder.find_package( package_name=package_name, package_directory=package_directory, diff --git a/tests/adaptors/packagefinder.py b/tests/adaptors/packagefinder.py index b242f26d..a44fe4d3 100644 --- a/tests/adaptors/packagefinder.py +++ b/tests/adaptors/packagefinder.py @@ -5,7 +5,7 @@ class BaseFakePackageFinder(AbstractPackageFinder): directory_map: dict[str, str] = {} - def determine_package_directory( + def determine_package_directories( self, package_name: str, file_system: AbstractFileSystem - ) -> str: - return self.directory_map[package_name] + ) -> set[str]: + return {self.directory_map[package_name]} diff --git a/tests/unit/adaptors/test_packagefinder.py b/tests/unit/adaptors/test_packagefinder.py index 51bb2eed..63494c6e 100644 --- a/tests/unit/adaptors/test_packagefinder.py +++ b/tests/unit/adaptors/test_packagefinder.py @@ -14,20 +14,20 @@ @pytest.mark.parametrize( "package, expected", ( - ("testpackage", assets / "testpackage"), + ("testpackage", {str(assets / "testpackage")}), ( "mynamespace.green", - assets / "namespacepackages" / "locationone" / "mynamespace" / "green", + {str(assets / "namespacepackages" / "locationone" / "mynamespace" / "green")}, ), ( "mynamespace.blue", - assets / "namespacepackages" / "locationtwo" / "mynamespace" / "blue", + {str(assets / "namespacepackages" / "locationtwo" / "mynamespace" / "blue")}, ), ), ) -def test_determine_package_directory(package, expected): - assert ImportLibPackageFinder().determine_package_directory(package, FileSystem()) == str( - expected +def test_determine_package_directories(package, expected): + assert ( + ImportLibPackageFinder().determine_package_directories(package, FileSystem()) == expected ) @@ -40,7 +40,7 @@ def test_determine_package_directory_doesnt_support_namespace_packages(): "namespace packages, adding an __init__.py file should fix the problem." ), ): - ImportLibPackageFinder().determine_package_directory("mynamespace", FakeFileSystem()) + ImportLibPackageFinder().determine_package_directories("mynamespace", FakeFileSystem()) @pytest.mark.parametrize( @@ -53,8 +53,8 @@ def test_determine_package_directory_doesnt_support_namespace_packages(): "mynamespace.yellow", ), ) -def test_determine_package_directory_doesnt_support_non_top_level_modules(package): +def test_determine_package_directories_doesnt_support_non_top_level_modules(package): with pytest.raises( exceptions.NotATopLevelModule, ): - ImportLibPackageFinder().determine_package_directory(package, FakeFileSystem()) + ImportLibPackageFinder().determine_package_directories(package, FakeFileSystem()) From 533bbddd6829e3c7ff97c3b309a971044ac6e13e Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 10:46:16 +0000 Subject: [PATCH 04/12] Return all directories from namespace packages This pushes the NamespacePackageEncountered error up the call stack into the use case. --- src/grimp/adaptors/packagefinder.py | 19 +++----------- tests/functional/test_error_handling.py | 13 --------- tests/unit/adaptors/test_packagefinder.py | 32 ++++++++++++++--------- 3 files changed, 23 insertions(+), 41 deletions(-) diff --git a/src/grimp/adaptors/packagefinder.py b/src/grimp/adaptors/packagefinder.py index 4115b60f..55c5d28f 100644 --- a/src/grimp/adaptors/packagefinder.py +++ b/src/grimp/adaptors/packagefinder.py @@ -12,10 +12,9 @@ class ImportLibPackageFinder(AbstractPackageFinder): - def _determine_package_directory( + def determine_package_directories( self, package_name: str, file_system: AbstractFileSystem - ) -> str: - # TODO - do we need to add the current working directory here? + ) -> set[str]: # Attempt to locate the package file. spec = importlib.util.find_spec(package_name) if not spec: @@ -26,18 +25,8 @@ def _determine_package_directory( if not self._is_a_package(spec, file_system) or self._has_a_non_namespace_parent(spec): raise exceptions.NotATopLevelModule - return file_system.dirname(spec.origin) - - raise exceptions.NamespacePackageEncountered( - f"Package '{package_name}' is a namespace package (see PEP 420). Try specifying the " - "portion name instead. If you are not intentionally using namespace packages, " - "adding an __init__.py file should fix the problem." - ) - - def determine_package_directories( - self, package_name: str, file_system: AbstractFileSystem - ) -> set[str]: - return {self._determine_package_directory(package_name, file_system)} + assert spec.submodule_search_locations # This should be the case if spec.has_location. + return set(spec.submodule_search_locations) def _is_a_package(self, spec: ModuleSpec, file_system: AbstractFileSystem) -> bool: assert spec.origin diff --git a/tests/functional/test_error_handling.py b/tests/functional/test_error_handling.py index 45635986..e79914ba 100644 --- a/tests/functional/test_error_handling.py +++ b/tests/functional/test_error_handling.py @@ -1,5 +1,4 @@ import os -import re import pytest # type: ignore @@ -19,15 +18,3 @@ def test_syntax_error_includes_module(): filename=filename, lineno=5, text="fromb . import two" ) assert expected_exception == excinfo.value - - -def test_missing_root_init_file(): - with pytest.raises( - exceptions.NamespacePackageEncountered, - match=re.escape( - "Package 'missingrootinitpackage' is a namespace package (see PEP 420). Try specifying " - "the portion name instead. If you are not intentionally " - "using namespace packages, adding an __init__.py file should fix the problem." - ), - ): - build_graph("missingrootinitpackage", cache_dir=None) diff --git a/tests/unit/adaptors/test_packagefinder.py b/tests/unit/adaptors/test_packagefinder.py index 63494c6e..f7f2de62 100644 --- a/tests/unit/adaptors/test_packagefinder.py +++ b/tests/unit/adaptors/test_packagefinder.py @@ -1,4 +1,3 @@ -import re from pathlib import Path import pytest # type: ignore @@ -15,6 +14,18 @@ "package, expected", ( ("testpackage", {str(assets / "testpackage")}), + ( + "missingrootinitpackage", + { + str(assets / "missingrootinitpackage"), + }, + ), + ( + "missingrootinitpackage.one", + { + str(assets / "missingrootinitpackage" / "one"), + }, + ), ( "mynamespace.green", {str(assets / "namespacepackages" / "locationone" / "mynamespace" / "green")}, @@ -23,6 +34,13 @@ "mynamespace.blue", {str(assets / "namespacepackages" / "locationtwo" / "mynamespace" / "blue")}, ), + ( + "mynamespace", + { + str(assets / "namespacepackages" / "locationone" / "mynamespace"), + str(assets / "namespacepackages" / "locationtwo" / "mynamespace"), + }, + ), ), ) def test_determine_package_directories(package, expected): @@ -31,18 +49,6 @@ def test_determine_package_directories(package, expected): ) -def test_determine_package_directory_doesnt_support_namespace_packages(): - with pytest.raises( - exceptions.NamespacePackageEncountered, - match=re.escape( - "Package 'mynamespace' is a namespace package (see PEP 420). Try specifying the" - " portion name instead. If you are not intentionally using " - "namespace packages, adding an __init__.py file should fix the problem." - ), - ): - ImportLibPackageFinder().determine_package_directories("mynamespace", FakeFileSystem()) - - @pytest.mark.parametrize( "package", ( From 3b91e4c3a205650b3ddb587590c9ddcfd009a19c Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 11:31:38 +0000 Subject: [PATCH 05/12] Adjust BaseFakePackageFinder to support multiple directories --- tests/adaptors/packagefinder.py | 6 ++++-- tests/unit/application/test_usecases.py | 8 ++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/adaptors/packagefinder.py b/tests/adaptors/packagefinder.py index a44fe4d3..5740677d 100644 --- a/tests/adaptors/packagefinder.py +++ b/tests/adaptors/packagefinder.py @@ -1,11 +1,13 @@ +from collections.abc import Mapping from grimp.application.ports.packagefinder import AbstractPackageFinder + from grimp.application.ports.filesystem import AbstractFileSystem class BaseFakePackageFinder(AbstractPackageFinder): - directory_map: dict[str, str] = {} + directory_map: Mapping[str, set[str]] = {} def determine_package_directories( self, package_name: str, file_system: AbstractFileSystem ) -> set[str]: - return {self.directory_map[package_name]} + return self.directory_map[package_name] diff --git a/tests/unit/application/test_usecases.py b/tests/unit/application/test_usecases.py index a6be720a..79b9cb0f 100644 --- a/tests/unit/application/test_usecases.py +++ b/tests/unit/application/test_usecases.py @@ -36,7 +36,7 @@ def test_happy_path(self, include_external_packages): ) class FakePackageFinder(BaseFakePackageFinder): - directory_map = {"mypackage": "/path/to/mypackage"} + directory_map = {"mypackage": {"/path/to/mypackage"}} with override_settings(FILE_SYSTEM=file_system, PACKAGE_FINDER=FakePackageFinder()): graph = usecases.build_graph( @@ -92,7 +92,7 @@ class FakePackageFinder(BaseFakePackageFinder): directory_map = { # TODO - PackageFinder assumes only a single directory per package. # This isn't the case for namespace packages. - "mypackage": "/path/to/mypackage" + "mypackage": {"/path/to/mypackage"} } with override_settings(FILE_SYSTEM=file_system, PACKAGE_FINDER=FakePackageFinder()): @@ -130,7 +130,7 @@ def test_build_graph_respects_cache_dir(self, supplied_cache_dir): file_system = FakeFileSystem() class FakePackageFinder(BaseFakePackageFinder): - directory_map = {"mypackage": "/path/to/mypackage"} + directory_map = {"mypackage": {"/path/to/mypackage"}} SOME_DEFAULT_CACHE_DIR = ".some_default" @@ -192,7 +192,7 @@ def test_forgives_wrong_type_being_passed_to_include_external_packages(self): ) class FakePackageFinder(BaseFakePackageFinder): - directory_map = {"mypackage": "/path/to/mypackage"} + directory_map = {"mypackage": {"/path/to/mypackage"}} with override_settings(FILE_SYSTEM=file_system, PACKAGE_FINDER=FakePackageFinder()): graph = usecases.build_graph( From 104f59d832977e92e0a4fcb47dcb4d3b508dc876 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 12:06:13 +0000 Subject: [PATCH 06/12] Correct wrong type being passed in test --- tests/unit/application/test_usecases.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/application/test_usecases.py b/tests/unit/application/test_usecases.py index 79b9cb0f..005b386d 100644 --- a/tests/unit/application/test_usecases.py +++ b/tests/unit/application/test_usecases.py @@ -199,7 +199,7 @@ class FakePackageFinder(BaseFakePackageFinder): "mypackage", # Note: this should be a bool, but we want to tolerate it, # as Import Linter currently has a bug where it will pass it as None. - include_external_packages=None, + include_external_packages=False, ) expected_modules = { From 205c68f8a235e16621f49478f5e0b8cccd75056f Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 12:23:14 +0000 Subject: [PATCH 07/12] MOVE LATER Loop through package directories --- src/grimp/application/usecases.py | 20 +++++------- tests/functional/test_namespace_packages.py | 34 ++++++++++++++++----- tests/unit/application/test_usecases.py | 9 +++--- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/src/grimp/application/usecases.py b/src/grimp/application/usecases.py index 7f544863..e0dedd12 100644 --- a/src/grimp/application/usecases.py +++ b/src/grimp/application/usecases.py @@ -13,7 +13,6 @@ from ..application.ports.modulefinder import AbstractModuleFinder, FoundPackage, ModuleFile from ..application.ports.packagefinder import AbstractPackageFinder from ..domain.valueobjects import DirectImport, Module -from .. import exceptions from .config import settings @@ -84,18 +83,13 @@ def _find_packages( package_directories = package_finder.determine_package_directories( package_name=package_name, file_system=file_system ) - try: - [package_directory] = package_directories - except ValueError: - # TODO handle multiple directories. - raise exceptions.NamespacePackageEncountered - - found_package = module_finder.find_package( - package_name=package_name, - package_directory=package_directory, - file_system=file_system, - ) - found_packages.add(found_package) + for package_directory in package_directories: + found_package = module_finder.find_package( + package_name=package_name, + package_directory=package_directory, + file_system=file_system, + ) + found_packages.add(found_package) return found_packages diff --git a/tests/functional/test_namespace_packages.py b/tests/functional/test_namespace_packages.py index 0d0d8b83..38df3709 100644 --- a/tests/functional/test_namespace_packages.py +++ b/tests/functional/test_namespace_packages.py @@ -1,6 +1,6 @@ import pytest # type: ignore -from grimp import build_graph, exceptions +from grimp import build_graph """ For ease of reference, these are the imports of all the files: @@ -17,14 +17,18 @@ nestednamespace.foo.alpha.blue.one, nestednamespace.bar.beta.orange """ +YELLOW_MODULES = {"mynamespace.yellow"} +GREEN_MODULES = {"mynamespace.green", "mynamespace.green.alpha"} +BLUE_MODULES = {"mynamespace.blue", "mynamespace.blue.alpha", "mynamespace.blue.beta"} + +@pytest.mark.xfail(strict=True) def test_build_graph_for_namespace(): - with pytest.raises(exceptions.NamespacePackageEncountered): - build_graph("mynamespace", cache_dir=None) + graph = build_graph("mynamespace", cache_dir=None) + assert graph.modules == {"mynamespace"} | YELLOW_MODULES | GREEN_MODULES | BLUE_MODULES -GREEN_MODULES = {"mynamespace.green", "mynamespace.green.alpha"} -BLUE_MODULES = {"mynamespace.blue", "mynamespace.blue.alpha", "mynamespace.blue.beta"} + assert graph.count_imports() @pytest.mark.parametrize( @@ -100,13 +104,29 @@ def test_import_between_namespace_children(): # Nested namespaces +@pytest.mark.xfail(strict=True) @pytest.mark.parametrize( "package_name", ("nestednamespace", "nestednamespace.foo", "nestednamespace.foo.alpha"), ) def test_build_graph_for_nested_namespace(package_name): - with pytest.raises(exceptions.NamespacePackageEncountered): - build_graph(package_name, cache_dir=None) + graph = build_graph(package_name, cache_dir=None) + + assert graph.modules == { + "nestednamespace", + "nestednamespace.foo", + "nestednamespace.foo.alpha", + "nestednamespace.foo.alpha.blue", + "nestednamespace.foo.alpha.blue.one", + "nestednamespace.foo.alpha.blue.two", + "nestednamespace.foo.alpha.green", + "nestednamespace.foo.alpha.green.one", + "nestednamespace.foo.alpha.green.two", + "nestednamespace.bar", + "nestednamespace.bar.beta", + "nestednamespace.bar.beta.orange", + } + assert graph.count_imports() @pytest.mark.parametrize( diff --git a/tests/unit/application/test_usecases.py b/tests/unit/application/test_usecases.py index 005b386d..716b4294 100644 --- a/tests/unit/application/test_usecases.py +++ b/tests/unit/application/test_usecases.py @@ -66,7 +66,7 @@ class FakePackageFinder(BaseFakePackageFinder): with pytest.raises(ValueError, match="Cannot find children of a squashed module."): graph.find_children(module) - @pytest.mark.xfail(reason="Not yet supported") + @pytest.mark.xfail(strict=True) def test_namespace_package_passed_as_root(self): file_system = FakeFileSystem( contents=""" @@ -90,9 +90,10 @@ def test_namespace_package_passed_as_root(self): class FakePackageFinder(BaseFakePackageFinder): directory_map = { - # TODO - PackageFinder assumes only a single directory per package. - # This isn't the case for namespace packages. - "mypackage": {"/path/to/mypackage"} + "mypackage": { + "/path/to/mypackage", + "/different-path/to/mypackage", + } } with override_settings(FILE_SYSTEM=file_system, PACKAGE_FINDER=FakePackageFinder()): From 41697e7702b50256b7621534cff2d998e1adfda8 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 13:11:42 +0000 Subject: [PATCH 08/12] Adjust ModuleFinder to support namespace packages --- src/grimp/adaptors/modulefinder.py | 3 +- tests/adaptors/filesystem.py | 23 +++++++++---- tests/unit/adaptors/test_modulefinder.py | 44 ++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/grimp/adaptors/modulefinder.py b/src/grimp/adaptors/modulefinder.py index 934190f9..2d08845c 100644 --- a/src/grimp/adaptors/modulefinder.py +++ b/src/grimp/adaptors/modulefinder.py @@ -40,7 +40,8 @@ def _get_python_files_inside_package(self, directory: str) -> Iterable[str]: for dirpath, dirs, files in self.file_system.walk(directory): # Don't include directories that aren't Python packages, # nor their subdirectories. - if "__init__.py" not in files: + current_dir_is_descendant = dirpath.startswith(directory) and dirpath != directory + if current_dir_is_descendant and "__init__.py" not in files: for d in list(dirs): dirs.remove(d) continue diff --git a/tests/adaptors/filesystem.py b/tests/adaptors/filesystem.py index d8ed455d..ed619764 100644 --- a/tests/adaptors/filesystem.py +++ b/tests/adaptors/filesystem.py @@ -68,12 +68,23 @@ def walk(self, directory_name): For each directory in the tree rooted at directory top (including top itself), it yields a 3-tuple (dirpath, dirnames, filenames). """ - try: - directory_contents = self.contents[directory_name] - except KeyError: - return [] - - yield from self._walk_contents(directory_contents, containing_directory=directory_name) + if contents_key := self._drill_down_for_contents_key(directory_name): + directory_contents = self.contents[contents_key] + assert isinstance(directory_contents, dict) + yield from self._walk_contents(directory_contents, containing_directory=contents_key) + + return [] + + def _drill_down_for_contents_key(self, directory_name: str) -> str | None: + if directory_name in self.contents: + return directory_name + + if self.sep in directory_name: + # Try one level up. + parent = self.sep.join(directory_name.split(self.sep)[:-1]) + return self._drill_down_for_contents_key(parent) + else: + return None def _walk_contents( self, directory_contents: dict[str, Any], containing_directory: str diff --git a/tests/unit/adaptors/test_modulefinder.py b/tests/unit/adaptors/test_modulefinder.py index 9d7a04c1..65c0f6eb 100644 --- a/tests/unit/adaptors/test_modulefinder.py +++ b/tests/unit/adaptors/test_modulefinder.py @@ -49,12 +49,52 @@ def test_happy_path(): ) -def test_namespaced_packages(): +def test_when_package_is_namespace(): module_finder = ModuleFinder() file_system = FakeFileSystem( contents=""" - /path/to/somenamespace/foo/ + /path/to/somenamespace/ + foo/ + __init__.py + blue.py + green/ + __init__.py + one.py + two/ + __init__.py + """ + ) + + result = module_finder.find_package( + package_name="somenamespace", + package_directory="/path/to/somenamespace", + file_system=file_system, + ) + + assert result == FoundPackage( + name="somenamespace", + directory="/path/to/somenamespace", + module_files=frozenset( + { + # N.B. no ModuleFile for the namespace package. + ModuleFile(module=Module("somenamespace.foo"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("somenamespace.foo.blue"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("somenamespace.foo.green"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("somenamespace.foo.green.one"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("somenamespace.foo.green.two"), mtime=DEFAULT_MTIME), + } + ), + ) + + +def test_when_package_is_namespace_portion(): + module_finder = ModuleFinder() + + file_system = FakeFileSystem( + contents=""" + /path/to/somenamespace/ + foo/ __init__.py blue.py green/ From 261c1ab656f71cd7ad705448bb1b9714e8594c94 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 14:21:13 +0000 Subject: [PATCH 09/12] Refactor ahead of including root package names in graph Also switch to using abstract types as parameters --- src/grimp/application/scanning.py | 4 ++-- src/grimp/application/usecases.py | 22 ++++++++++++---------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/grimp/application/scanning.py b/src/grimp/application/scanning.py index 09eab3c8..da111796 100644 --- a/src/grimp/application/scanning.py +++ b/src/grimp/application/scanning.py @@ -1,4 +1,4 @@ -from collections.abc import Collection +from collections.abc import Collection, Set from grimp import _rustgrimp as rust # type: ignore[attr-defined] from grimp.domain.valueobjects import DirectImport, Module @@ -10,7 +10,7 @@ def scan_imports( module_files: Collection[ModuleFile], *, - found_packages: set[FoundPackage], + found_packages: Set[FoundPackage], include_external_packages: bool, exclude_type_checking_imports: bool, ) -> dict[ModuleFile, set[DirectImport]]: diff --git a/src/grimp/application/usecases.py b/src/grimp/application/usecases.py index e0dedd12..b5b8d585 100644 --- a/src/grimp/application/usecases.py +++ b/src/grimp/application/usecases.py @@ -4,7 +4,7 @@ from typing import cast -from collections.abc import Sequence, Iterable +from collections.abc import Sequence, Iterable, Set, Mapping from .scanning import scan_imports from ..application.ports import caching @@ -51,9 +51,10 @@ def build_graph( """ file_system: AbstractFileSystem = settings.FILE_SYSTEM + root_package_names = [package_name] + list(additional_package_names) found_packages = _find_packages( file_system=file_system, - package_names=[package_name] + list(additional_package_names), + root_package_names=root_package_names, ) imports_by_module = _scan_packages( @@ -64,22 +65,22 @@ def build_graph( cache_dir=cache_dir, ) - graph = _assemble_graph(found_packages, imports_by_module) + graph = _assemble_graph(root_package_names, found_packages, imports_by_module) return graph def _find_packages( - file_system: AbstractFileSystem, package_names: Sequence[object] + file_system: AbstractFileSystem, root_package_names: Sequence[object] ) -> set[FoundPackage]: - package_names = _validate_package_names_are_strings(package_names) + root_package_names = _validate_package_names_are_strings(root_package_names) module_finder: AbstractModuleFinder = settings.MODULE_FINDER package_finder: AbstractPackageFinder = settings.PACKAGE_FINDER found_packages: set[FoundPackage] = set() - for package_name in package_names: + for package_name in root_package_names: package_directories = package_finder.determine_package_directories( package_name=package_name, file_system=file_system ) @@ -103,7 +104,7 @@ def _validate_package_names_are_strings( def _scan_packages( - found_packages: set[FoundPackage], + found_packages: Set[FoundPackage], file_system: BasicFileSystem, include_external_packages: bool, exclude_type_checking_imports: bool, @@ -152,8 +153,9 @@ def _scan_packages( def _assemble_graph( - found_packages: set[FoundPackage], - imports_by_module: dict[Module, set[DirectImport]], + root_package_names: Sequence[str], + found_packages: Set[FoundPackage], + imports_by_module: Mapping[Module, Set[DirectImport]], ) -> ImportGraph: graph: ImportGraph = settings.IMPORT_GRAPH_CLASS() @@ -178,7 +180,7 @@ def _assemble_graph( return graph -def _is_external(module: Module, package_modules: set[Module]) -> bool: +def _is_external(module: Module, package_modules: Set[Module]) -> bool: return not any( module.is_descendant_of(package_module) or module == package_module for package_module in package_modules From 0dac88b338f0403216703f33e16e6fe1cce7ded4 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 14:32:33 +0000 Subject: [PATCH 10/12] Got building working for non-nested namespace packages --- src/grimp/application/usecases.py | 5 +++++ tests/functional/test_namespace_packages.py | 1 - tests/unit/application/test_usecases.py | 1 - 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/grimp/application/usecases.py b/src/grimp/application/usecases.py index b5b8d585..ef3225d8 100644 --- a/src/grimp/application/usecases.py +++ b/src/grimp/application/usecases.py @@ -161,6 +161,11 @@ def _assemble_graph( package_modules = {Module(found_package.name) for found_package in found_packages} + # Add root packages first. These will usually be the names of the found packages, but + # not in the case of namespace packages. + for root_package_name in root_package_names: + graph.add_module(root_package_name) + for module, direct_imports in imports_by_module.items(): graph.add_module(module.name) for direct_import in direct_imports: diff --git a/tests/functional/test_namespace_packages.py b/tests/functional/test_namespace_packages.py index 38df3709..8066b44c 100644 --- a/tests/functional/test_namespace_packages.py +++ b/tests/functional/test_namespace_packages.py @@ -22,7 +22,6 @@ BLUE_MODULES = {"mynamespace.blue", "mynamespace.blue.alpha", "mynamespace.blue.beta"} -@pytest.mark.xfail(strict=True) def test_build_graph_for_namespace(): graph = build_graph("mynamespace", cache_dir=None) diff --git a/tests/unit/application/test_usecases.py b/tests/unit/application/test_usecases.py index 716b4294..88b61588 100644 --- a/tests/unit/application/test_usecases.py +++ b/tests/unit/application/test_usecases.py @@ -66,7 +66,6 @@ class FakePackageFinder(BaseFakePackageFinder): with pytest.raises(ValueError, match="Cannot find children of a squashed module."): graph.find_children(module) - @pytest.mark.xfail(strict=True) def test_namespace_package_passed_as_root(self): file_system = FakeFileSystem( contents=""" From eb277f5dee568467ad5e7c7e0605832860ab44a2 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 14:57:19 +0000 Subject: [PATCH 11/12] Correct module finder's handling of nested namespace packages --- src/grimp/adaptors/modulefinder.py | 30 +++++++++++---- tests/unit/adaptors/test_modulefinder.py | 47 ++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/src/grimp/adaptors/modulefinder.py b/src/grimp/adaptors/modulefinder.py index 2d08845c..310a6ea1 100644 --- a/src/grimp/adaptors/modulefinder.py +++ b/src/grimp/adaptors/modulefinder.py @@ -4,6 +4,7 @@ from grimp.application.ports import modulefinder from grimp.application.ports.filesystem import AbstractFileSystem from grimp.domain.valueobjects import Module +from collections.abc import Set logger = logging.getLogger(__name__) @@ -37,14 +38,23 @@ def _get_python_files_inside_package(self, directory: str) -> Iterable[str]: Return: Generator of Python file names. """ + # Containers are directories within `directory` that contain an __init__.py + # and who do not have any ancestors within `directory` that do contain an __init__.py. + # We keep track of these so we can tell the difference between nested namespace packages + # (which we do want to include) and orphaned packages (which we don't). + containers: set[str] = set() for dirpath, dirs, files in self.file_system.walk(directory): - # Don't include directories that aren't Python packages, - # nor their subdirectories. - current_dir_is_descendant = dirpath.startswith(directory) and dirpath != directory - if current_dir_is_descendant and "__init__.py" not in files: - for d in list(dirs): - dirs.remove(d) - continue + if self._is_in_container(dirpath, containers): + # Are we somewhere inside a non-namespace package (i.e. portion)? If so, don't include directories + # that aren't Python packages, nor their subdirectories. + if "__init__.py" not in files: + # Don't drill down further in this directory. + for d in list(dirs): + dirs.remove(d) + continue + elif "__init__.py" in files: + # It's a container. + containers.add(dirpath) # Don't include hidden directories. dirs_to_remove = [d for d in dirs if self._should_ignore_dir(d)] @@ -55,6 +65,12 @@ def _get_python_files_inside_package(self, directory: str) -> Iterable[str]: if self._is_python_file(filename, dirpath): yield self.file_system.join(dirpath, filename) + def _is_in_container(self, directory: str, containers: Set[str]) -> bool: + for container in containers: + if directory.startswith(container): + return True + return False + def _should_ignore_dir(self, directory: str) -> bool: # TODO: make this configurable. # Skip adding directories that are hidden. diff --git a/tests/unit/adaptors/test_modulefinder.py b/tests/unit/adaptors/test_modulefinder.py index 65c0f6eb..72199b35 100644 --- a/tests/unit/adaptors/test_modulefinder.py +++ b/tests/unit/adaptors/test_modulefinder.py @@ -164,6 +164,53 @@ def test_ignores_orphaned_python_files(): ) +def test_includes_nested_namespace_packages_but_not_orphans(): + module_finder = ModuleFinder() + + file_system = FakeFileSystem( + contents=""" + /path/to/mypackage/ + foo/ + blue/ + __init__.py + one.py + two/ + __init__.py + alpha.py + noinitpackage/ + three.py + orphan/ + __init__.py + four.py + green/ + five/ + __init__.py + beta.py + """ + ) + + result = module_finder.find_package( + package_name="mypackage", + package_directory="/path/to/mypackage", + file_system=file_system, + ) + + assert result == FoundPackage( + name="mypackage", + directory="/path/to/mypackage", + module_files=frozenset( + { + ModuleFile(module=Module("mypackage.foo.blue"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("mypackage.foo.blue.one"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("mypackage.foo.blue.two"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("mypackage.foo.blue.two.alpha"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("mypackage.foo.green.five"), mtime=DEFAULT_MTIME), + ModuleFile(module=Module("mypackage.foo.green.five.beta"), mtime=DEFAULT_MTIME), + } + ), + ) + + @pytest.mark.parametrize( "extension, should_warn", ( From 0fcea6d3c0a88cd498f6062ce753322980f28589 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Wed, 3 Dec 2025 15:55:16 +0000 Subject: [PATCH 12/12] TMP: [skip ci]