From 8a58ef1df7fdc8fc780ac35c4ee4156a4103a3f9 Mon Sep 17 00:00:00 2001 From: Eugene Toder Date: Wed, 20 Mar 2024 21:53:41 -0400 Subject: [PATCH 1/6] Fix discovery of modules in namespace packages Instead of relying on `__init__.py` files, stop at the first parent directory that is in `sys.path`. This gives the shortest module name under which the file can really be imported. (Unless there are name conflicts in `sys.path`, which is arguably a misconfiguration; this is caught by the location check in `module_tree`.) --- src/slotscheck/discovery.py | 15 ++++++++++----- tests/src/conftest.py | 4 ++-- tests/src/test_discovery.py | 24 +++++++++++++++--------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/slotscheck/discovery.py b/src/slotscheck/discovery.py index 1009328..d007e92 100644 --- a/src/slotscheck/discovery.py +++ b/src/slotscheck/discovery.py @@ -2,6 +2,7 @@ import importlib import pkgutil +import sys from dataclasses import dataclass, field, replace from functools import partial, reduce from importlib.util import find_spec @@ -299,15 +300,19 @@ def _is_package(p: AbsPath) -> bool: return p.is_dir() and (p / _INIT_PY).is_file() -def find_modules(p: AbsPath) -> Iterable[ModuleLocated]: +def find_modules( + p: AbsPath, python_path: Optional[FrozenSet[AbsPath]] = None +) -> Iterable[ModuleLocated]: "Recursively find modules at given path. Nonexistent Path is ignored" + if python_path is None: + python_path = frozenset(map(Path, sys.path)) if p.name == _INIT_PY: - yield from find_modules(p.parent) + yield from find_modules(p.parent, python_path) elif _is_module(p): - parents = [p] + list(takewhile(_is_package, p.parents)) + parents = [p, *takewhile(lambda p: p not in python_path, p.parents)] yield ModuleLocated( ".".join(p.stem for p in reversed(parents)), - (p / "__init__.py" if _is_package(p) else p), + (p / _INIT_PY if _is_package(p) else p), ) elif p.is_dir(): - yield from flatten(map(find_modules, p.iterdir())) + yield from flatten(find_modules(sp, python_path) for sp in p.iterdir()) diff --git a/tests/src/conftest.py b/tests/src/conftest.py index 732fd4d..32ebd9f 100644 --- a/tests/src/conftest.py +++ b/tests/src/conftest.py @@ -13,9 +13,9 @@ @pytest.fixture(scope="session", autouse=True) def add_pypath() -> Iterator[None]: "Add example modules to the python path" - sys.path.insert(0, str(EXAMPLES_DIR)) + sys.path[:0] = [str(EXAMPLES_DIR), str(EXAMPLES_DIR / "other")] yield - sys.path.remove(str(EXAMPLES_DIR)) + del sys.path[:2] @pytest.fixture(autouse=True) diff --git a/tests/src/test_discovery.py b/tests/src/test_discovery.py index 769b795..bba13ce 100644 --- a/tests/src/test_discovery.py +++ b/tests/src/test_discovery.py @@ -370,13 +370,13 @@ def test_given_nonpython_file(self): def test_given_python_file(self): location = EXAMPLES_DIR / "files/subdir/myfile.py" result = list(find_modules(location)) - assert result == [ModuleLocated("myfile", location)] + assert result == [ModuleLocated("files.subdir.myfile", location)] def test_given_python_root_module(self): location = EXAMPLES_DIR / "files/subdir/some_module/" result = list(find_modules(location)) assert result == [ - ModuleLocated("some_module", location / "__init__.py") + ModuleLocated("files.subdir.some_module", location / "__init__.py") ] def test_given_dir_containing_python_files(self): @@ -384,10 +384,12 @@ def test_given_dir_containing_python_files(self): result = list(find_modules(location)) assert len(result) == 4 assert set(result) == { - ModuleLocated("bla", location / "bla.py"), - ModuleLocated("foo", location / "foo.py"), - ModuleLocated("foo", location / "sub/foo.py"), - ModuleLocated("mymodule", location / "mymodule/__init__.py"), + ModuleLocated("files.my_scripts.bla", location / "bla.py"), + ModuleLocated("files.my_scripts.foo", location / "foo.py"), + ModuleLocated("files.my_scripts.sub.foo", location / "sub/foo.py"), + ModuleLocated( + "files.my_scripts.mymodule", location / "mymodule/__init__.py" + ), } def test_given_file_within_module(self): @@ -395,7 +397,7 @@ def test_given_file_within_module(self): result = list(find_modules(location)) assert result == [ ModuleLocated( - "some_module.sub.foo", + "files.subdir.some_module.sub.foo", EXAMPLES_DIR / "files/subdir/some_module/sub/foo.py", ) ] @@ -404,13 +406,17 @@ def test_given_submodule(self): location = EXAMPLES_DIR / "files/subdir/some_module/sub" result = list(find_modules(location)) assert result == [ - ModuleLocated("some_module.sub", location / "__init__.py") + ModuleLocated( + "files.subdir.some_module.sub", location / "__init__.py" + ) ] def test_given_init_py(self): location = EXAMPLES_DIR / "files/subdir/some_module/sub/__init__.py" result = list(find_modules(location)) - assert result == [ModuleLocated("some_module.sub", location)] + assert result == [ + ModuleLocated("files.subdir.some_module.sub", location) + ] class TestConsolidate: From 682c6887e060993fa5ebbdd5a7b4b07cb4d6b158 Mon Sep 17 00:00:00 2001 From: Eugene Toder Date: Thu, 21 Mar 2024 17:03:08 -0400 Subject: [PATCH 2/6] Address review comments --- src/slotscheck/discovery.py | 31 ++++++++++++++++++++++--------- tests/src/test_cli.py | 10 ++++++++++ tests/src/test_discovery.py | 6 ++++++ 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/slotscheck/discovery.py b/src/slotscheck/discovery.py index d007e92..8428533 100644 --- a/src/slotscheck/discovery.py +++ b/src/slotscheck/discovery.py @@ -7,7 +7,7 @@ from functools import partial, reduce from importlib.util import find_spec from inspect import isclass -from itertools import chain, takewhile +from itertools import chain from pathlib import Path from textwrap import indent from types import ModuleType @@ -300,19 +300,32 @@ def _is_package(p: AbsPath) -> bool: return p.is_dir() and (p / _INIT_PY).is_file() -def find_modules( - p: AbsPath, python_path: Optional[FrozenSet[AbsPath]] = None +def _module_parents( + p: AbsPath, sys_path: FrozenSet[AbsPath] +) -> Iterable[AbsPath]: + yield p + for pp in p.parents: + if pp in sys_path: + return + yield pp + raise ValueError(f"File {p} is outside of PYTHONPATH ({sys.path})") + + +def _find_modules( + p: AbsPath, sys_path: FrozenSet[AbsPath] ) -> Iterable[ModuleLocated]: - "Recursively find modules at given path. Nonexistent Path is ignored" - if python_path is None: - python_path = frozenset(map(Path, sys.path)) if p.name == _INIT_PY: - yield from find_modules(p.parent, python_path) + yield from _find_modules(p.parent, sys_path) elif _is_module(p): - parents = [p, *takewhile(lambda p: p not in python_path, p.parents)] + parents = list(_module_parents(p, sys_path)) yield ModuleLocated( ".".join(p.stem for p in reversed(parents)), (p / _INIT_PY if _is_package(p) else p), ) elif p.is_dir(): - yield from flatten(find_modules(sp, python_path) for sp in p.iterdir()) + yield from flatten(_find_modules(cp, sys_path) for cp in p.iterdir()) + + +def find_modules(p: AbsPath) -> Iterable[ModuleLocated]: + "Recursively find modules at given path. Nonexistent Path is ignored" + return _find_modules(p, frozenset(map(Path, sys.path))) diff --git a/tests/src/test_cli.py b/tests/src/test_cli.py index 5497da4..9728807 100644 --- a/tests/src/test_cli.py +++ b/tests/src/test_cli.py @@ -157,6 +157,16 @@ def test_multiple_modules(runner: CliRunner): assert result.output == "All OK!\nScanned 11 module(s), 70 class(es).\n" +def test_implicitly_namespaced_path(runner: CliRunner): + result = runner.invoke( + cli, + [str(EXAMPLES_DIR / "implicitly_namespaced")], + catch_exceptions=False, + ) + assert result.exit_code == 0 + assert result.output == "All OK!\nScanned 7 module(s), 1 class(es).\n" + + def test_multiple_paths(runner: CliRunner): result = runner.invoke( cli, diff --git a/tests/src/test_discovery.py b/tests/src/test_discovery.py index bba13ce..b90b9b5 100644 --- a/tests/src/test_discovery.py +++ b/tests/src/test_discovery.py @@ -418,6 +418,12 @@ def test_given_init_py(self): ModuleLocated("files.subdir.some_module.sub", location) ] + def test_given_file_not_in_sys_path(self, tmp_path): + location = tmp_path / "foo.py" + location.touch() + with pytest.raises(ValueError, match="is outside of PYTHONPATH"): + list(find_modules(location)) + class TestConsolidate: def test_empty(self): From 9d6ae693c8fa0331042706c0f13b56dcfb6d1bdb Mon Sep 17 00:00:00 2001 From: Eugene Toder Date: Fri, 22 Mar 2024 13:18:28 -0400 Subject: [PATCH 3/6] Update documentation --- docs/advanced.rst | 8 -------- 1 file changed, 8 deletions(-) diff --git a/docs/advanced.rst b/docs/advanced.rst index c755ecc..817de50 100644 --- a/docs/advanced.rst +++ b/docs/advanced.rst @@ -51,11 +51,3 @@ Use the following configuration: # This requires `slotscheck` to be installed in that environment. # # language: system - - -Namespace packages ------------------- - -Namespace packages come in `different flavors `_. -When using the ``-m/--module`` flag in the CLI, all these flavors are supported. -When specifying file paths, *native* namespace packages are not supported. From 8626cc2ee7a82aa67f17962f4afec0e9d4204a29 Mon Sep 17 00:00:00 2001 From: Arie Bovenberg Date: Sun, 24 Mar 2024 22:15:45 +0100 Subject: [PATCH 4/6] improve error message on module not found on path --- src/slotscheck/discovery.py | 2 +- tests/src/test_cli.py | 13 +++++++++++++ tests/src/test_discovery.py | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/slotscheck/discovery.py b/src/slotscheck/discovery.py index 8428533..6b6fb72 100644 --- a/src/slotscheck/discovery.py +++ b/src/slotscheck/discovery.py @@ -308,7 +308,7 @@ def _module_parents( if pp in sys_path: return yield pp - raise ValueError(f"File {p} is outside of PYTHONPATH ({sys.path})") + raise ModuleNotFoundError(f"No module named '{p.stem}'", name=p.stem) def _find_modules( diff --git a/tests/src/test_cli.py b/tests/src/test_cli.py index 9728807..7e54007 100644 --- a/tests/src/test_cli.py +++ b/tests/src/test_cli.py @@ -39,6 +39,19 @@ def test_module_doesnt_exist(runner: CliRunner): ) +def test_python_file_not_in_sys_path(runner: CliRunner, tmpdir): + file = tmpdir / "foo.py" + file.write_text('print("Hello, world!")', encoding="utf-8") + result = runner.invoke(cli, [str(file)]) + assert result.exit_code == 1 + assert isinstance(result.exception, SystemExit) + assert result.output == ( + "ERROR: Module 'foo' not found.\n\n" + "See slotscheck.rtfd.io/en/latest/discovery.html\n" + "for help resolving common import problems.\n" + ) + + def test_module_is_uninspectable(runner: CliRunner): result = runner.invoke(cli, ["-m", "broken.submodule"]) assert result.exit_code == 1 diff --git a/tests/src/test_discovery.py b/tests/src/test_discovery.py index b90b9b5..f0c2488 100644 --- a/tests/src/test_discovery.py +++ b/tests/src/test_discovery.py @@ -421,7 +421,7 @@ def test_given_init_py(self): def test_given_file_not_in_sys_path(self, tmp_path): location = tmp_path / "foo.py" location.touch() - with pytest.raises(ValueError, match="is outside of PYTHONPATH"): + with pytest.raises(ModuleNotFoundError, match="foo"): list(find_modules(location)) From 908f6d4e7329a862a506e2ce05bb59ed6d8c5455 Mon Sep 17 00:00:00 2001 From: Eugene Toder Date: Mon, 25 Mar 2024 09:04:59 -0400 Subject: [PATCH 5/6] Address review comments --- src/slotscheck/cli.py | 5 +++-- src/slotscheck/discovery.py | 10 ++++++++-- tests/src/test_cli.py | 14 ++++++++------ tests/src/test_discovery.py | 6 ++++-- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/slotscheck/cli.py b/src/slotscheck/cli.py index eb5b9fe..41c7d9d 100644 --- a/src/slotscheck/cli.py +++ b/src/slotscheck/cli.py @@ -42,6 +42,7 @@ from .discovery import ( AbsPath, FailedImport, + FileNotInSysPathError, ModuleLocated, ModuleName, ModuleTree, @@ -162,9 +163,9 @@ def root( try: classes, modules = _collect(files, module, conf) - except ModuleNotFoundError as e: + except (ModuleNotFoundError, FileNotInSysPathError) as e: print( - f"ERROR: Module '{e.name}' not found.\n\n" + f"ERROR: {e}.\n\n" "See slotscheck.rtfd.io/en/latest/discovery.html\n" "for help resolving common import problems." ) diff --git a/src/slotscheck/discovery.py b/src/slotscheck/discovery.py index 6b6fb72..badae77 100644 --- a/src/slotscheck/discovery.py +++ b/src/slotscheck/discovery.py @@ -165,7 +165,7 @@ def module_tree( except BaseException as e: return FailedImport(module, e) if spec is None: - raise ModuleNotFoundError(f"No module named '{module}'", name=module) + raise ModuleNotFoundError(f"No module named {module!r}", name=module) *namespaces, name = module.split(".") location = Path(spec.origin) if spec.has_location and spec.origin else None tree: ModuleTree @@ -292,6 +292,12 @@ class ModuleLocated(NamedTuple): expected_location: Optional[AbsPath] +class FileNotInSysPathError(Exception): + def __init__(self, file: Path) -> None: + super().__init__(f"File {str(file)!r} is not in PYTHONPATH") + self.file = file + + def _is_module(p: AbsPath) -> bool: return (p.is_file() and p.suffixes == [".py"]) or _is_package(p) @@ -308,7 +314,7 @@ def _module_parents( if pp in sys_path: return yield pp - raise ModuleNotFoundError(f"No module named '{p.stem}'", name=p.stem) + raise FileNotInSysPathError(p) def _find_modules( diff --git a/tests/src/test_cli.py b/tests/src/test_cli.py index 7e54007..a1edbd3 100644 --- a/tests/src/test_cli.py +++ b/tests/src/test_cli.py @@ -1,4 +1,5 @@ import os +import re from importlib.util import find_spec from pathlib import Path @@ -33,22 +34,23 @@ def test_module_doesnt_exist(runner: CliRunner): assert result.exit_code == 1 assert isinstance(result.exception, SystemExit) assert result.output == ( - "ERROR: Module 'foo' not found.\n\n" + "ERROR: No module named 'foo'.\n\n" "See slotscheck.rtfd.io/en/latest/discovery.html\n" "for help resolving common import problems.\n" ) -def test_python_file_not_in_sys_path(runner: CliRunner, tmpdir): - file = tmpdir / "foo.py" +def test_python_file_not_in_sys_path(runner: CliRunner, tmp_path: Path): + file = tmp_path / "foo.py" file.write_text('print("Hello, world!")', encoding="utf-8") result = runner.invoke(cli, [str(file)]) assert result.exit_code == 1 assert isinstance(result.exception, SystemExit) - assert result.output == ( - "ERROR: Module 'foo' not found.\n\n" + assert re.fullmatch( + "ERROR: File '.*/foo.py' is not in PYTHONPATH.\n\n" "See slotscheck.rtfd.io/en/latest/discovery.html\n" - "for help resolving common import problems.\n" + "for help resolving common import problems.\n", + result.output, ) diff --git a/tests/src/test_discovery.py b/tests/src/test_discovery.py index f0c2488..574f9f4 100644 --- a/tests/src/test_discovery.py +++ b/tests/src/test_discovery.py @@ -1,3 +1,4 @@ +from pathlib import Path from typing import FrozenSet, List, TypeVar from unittest import mock @@ -5,6 +6,7 @@ from slotscheck.discovery import ( FailedImport, + FileNotInSysPathError, Module, ModuleLocated, ModuleTree, @@ -418,10 +420,10 @@ def test_given_init_py(self): ModuleLocated("files.subdir.some_module.sub", location) ] - def test_given_file_not_in_sys_path(self, tmp_path): + def test_given_file_not_in_sys_path(self, tmp_path: Path): location = tmp_path / "foo.py" location.touch() - with pytest.raises(ModuleNotFoundError, match="foo"): + with pytest.raises(FileNotInSysPathError, match=r"foo\.py"): list(find_modules(location)) From 9f60d08411316b8f5e1894ff62cac475fc10d3f2 Mon Sep 17 00:00:00 2001 From: Arie Bovenberg Date: Mon, 25 Mar 2024 20:14:03 +0100 Subject: [PATCH 6/6] Amend docs about subdirectory module discovery --- docs/discovery.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/discovery.rst b/docs/discovery.rst index 3b1839e..370df01 100644 --- a/docs/discovery.rst +++ b/docs/discovery.rst @@ -9,7 +9,7 @@ However, there are some complications that you may need to be aware of. You should generally be fine if you follow these rules: - - To check files in your current directory, + - To check files in your current directory, or subdirectories of it, you should run slotscheck as ``python -m slotscheck``. - To check files elsewhere, you may need to set the ``$PYTHONPATH`` environment variable.