Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix discovery of modules in namespace packages #228

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions docs/advanced.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://packaging.python.org/en/latest/guides/packaging-namespace-packages/>`_.
When using the ``-m/--module`` flag in the CLI, all these flavors are supported.
When specifying file paths, *native* namespace packages are not supported.
32 changes: 25 additions & 7 deletions src/slotscheck/discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@

import importlib
import pkgutil
import sys
from dataclasses import dataclass, field, replace
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
Expand Down Expand Up @@ -299,15 +300,32 @@ def _is_package(p: AbsPath) -> bool:
return p.is_dir() and (p / _INIT_PY).is_file()


def find_modules(p: AbsPath) -> Iterable[ModuleLocated]:
"Recursively find modules at given path. Nonexistent Path is ignored"
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 ModuleNotFoundError(f"No module named '{p.stem}'", name=p.stem)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you want to reuse the logic in cli.py, but this error can be very misleading. ModuleNotFoundError means we could not resolve a "dotted.module.name". Here we could not find a module name for a file path. We were going in the opposite direction (file -> module instead of module -> file), which does not normally happen in Python. I suggest that we add a new exception type for this, keep the full file path, and handle it in cli.py similar to ModuleNotFoundError.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, let's do a custom exception 👍



def _find_modules(
p: AbsPath, sys_path: FrozenSet[AbsPath]
) -> Iterable[ModuleLocated]:
if p.name == _INIT_PY:
yield from find_modules(p.parent)
yield from _find_modules(p.parent, sys_path)
elif _is_module(p):
parents = [p] + list(takewhile(_is_package, 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),
(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(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)))
4 changes: 2 additions & 2 deletions tests/src/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 23 additions & 0 deletions tests/src/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like tmp_path is now recommended over tmpdir: https://docs.pytest.org/en/latest/how-to/tmp_path.html

It returns pathlib.Path instead of the legacy py.path.local.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about this! Excellent! love pathlib

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
Expand Down Expand Up @@ -157,6 +170,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,
Expand Down
30 changes: 21 additions & 9 deletions tests/src/test_discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,32 +370,34 @@ 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):
location = EXAMPLES_DIR / "files/my_scripts/"
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):
location = EXAMPLES_DIR / "files/subdir/some_module/sub/foo.py"
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",
)
]
Expand All @@ -404,13 +406,23 @@ 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)
]

def test_given_file_not_in_sys_path(self, tmp_path):
location = tmp_path / "foo.py"
location.touch()
with pytest.raises(ModuleNotFoundError, match="foo"):
list(find_modules(location))


class TestConsolidate:
Expand Down
Loading