diff --git a/src/osw/controller/page_package.py b/src/osw/controller/page_package.py index da82712..c84e7b5 100644 --- a/src/osw/controller/page_package.py +++ b/src/osw/controller/page_package.py @@ -248,6 +248,32 @@ def find_package_dir( return matching_dirs[0] +def find_first_package_dir( + package_or_script_name: str, search_paths: List[Path] = None +) -> Optional[Path]: + """searches the search_paths in order and returns the first match + + Unlike find_package_dir, which searches all paths at once and rejects a + name that exists in more than one of them, this prefers the earlier path. + Returns None if no search path holds the name. A path holding more than one + match is ambiguous on its own, so it is skipped with a warning. + """ + if search_paths is None: + search_paths = [] + for search_path in search_paths: + try: + return find_package_dir(package_or_script_name, [search_path]) + except FileNotFoundError: + # expected: not every search path holds every package + continue + except ValueError as e: + warn( + f"Multiple elements {package_or_script_name} found in " + f"{search_path}: {e}" + ) + return None + + def get_listed_pages_from_package_info(package_info: Union[dict, Path]) -> List[str]: """Takes in the output of read_package_info_file and returns a list of pages listed in the package""" @@ -663,35 +689,58 @@ def recursive( if params.read_listed_pages_from_script: search_paths = [params.script_dir] search_paths.extend(params.additional_script_dirs) - try: - script_path = find_package_dir( - f"{package_to_process}.py", search_paths - ) - package_script = read_package_script_file(script_path) - new_listed_pages = get_listed_pages_from_package_script( - package_script - ) - required_packages = get_required_packages_from_package_script( - package_script + # Prefer the script in script_dir over the additional dirs, + # instead of failing on a script present in several. + script_path = find_first_package_dir( + f"{package_to_process}.py", search_paths + ) + new_listed_pages = [] + required_packages = [] + if script_path is None: + warn( + f"Package script for {package_to_process} not found in any " + f"of the search paths: {search_paths}" ) - except Exception as e: - warn(f"Error reading package script for {package_to_process}: {e}") - new_listed_pages = [] - required_packages = [] + else: + try: + package_script = read_package_script_file(script_path) + new_listed_pages = get_listed_pages_from_package_script( + package_script + ) + required_packages = get_required_packages_from_package_script( + package_script + ) + except Exception as e: + warn( + f"Error reading package script for " + f"{package_to_process}: {e}" + ) else: search_paths = [params.creation_config.working_dir.parent] search_paths.extend(params.additional_package_dirs) - try: - package_dir = find_package_dir(package_to_process, search_paths) - package_info = read_package_info_file(package_dir) - new_listed_pages = get_listed_pages_from_package_info(package_info) - required_packages = get_required_packages_from_package_info_file( - package_info + # Prefer the package in the working dir over the additional + # dirs, instead of failing on a package present in several. + package_dir = find_first_package_dir(package_to_process, search_paths) + new_listed_pages = [] + required_packages = [] + if package_dir is None: + warn( + f"Package info for {package_to_process} not found in any " + f"of the search paths: {search_paths}" ) - except Exception as e: - warn(f"Error reading package info for {package_to_process}: {e}") - new_listed_pages = [] - required_packages = [] + else: + try: + package_info = read_package_info_file(package_dir) + new_listed_pages = get_listed_pages_from_package_info( + package_info + ) + required_packages = ( + get_required_packages_from_package_info_file(package_info) + ) + except Exception as e: + warn( + f"Error reading package info for {package_to_process}: {e}" + ) # Check for redundant pages for pack_ in listed_pages.keys(): new_redundant_pages = list( diff --git a/tests/test_page_package_search.py b/tests/test_page_package_search.py new file mode 100644 index 0000000..24f20b1 --- /dev/null +++ b/tests/test_page_package_search.py @@ -0,0 +1,94 @@ +"""Unit tests for package directory lookup in osw.controller.page_package. + +Regression guard for #135: a package present in both the working dir and an +additional package dir must resolve to the working dir one, instead of raising +because the name was found more than once. +""" + +import pytest + +from osw.controller.page_package import find_first_package_dir, find_package_dir + + +@pytest.fixture +def two_dirs(tmp_path): + """A working dir and an additional dir, both holding 'MyPackage'.""" + work_dir = tmp_path / "work" + extra_dir = tmp_path / "extra" + (work_dir / "MyPackage").mkdir(parents=True) + (extra_dir / "MyPackage").mkdir(parents=True) + return work_dir, extra_dir + + +def test_prefers_the_first_search_path(two_dirs): + work_dir, extra_dir = two_dirs + + found = find_first_package_dir("MyPackage", [work_dir, extra_dir]) + + assert found == work_dir / "MyPackage" + + +def test_search_order_decides(two_dirs): + """The same two dirs in the other order resolve to the other package.""" + work_dir, extra_dir = two_dirs + + found = find_first_package_dir("MyPackage", [extra_dir, work_dir]) + + assert found == extra_dir / "MyPackage" + + +def test_falls_through_to_a_later_path(two_dirs, tmp_path): + _, extra_dir = two_dirs + empty_dir = tmp_path / "empty" + empty_dir.mkdir() + + found = find_first_package_dir("MyPackage", [empty_dir, extra_dir]) + + assert found == extra_dir / "MyPackage" + + +def test_returns_none_when_nothing_matches(tmp_path): + assert find_first_package_dir("MyPackage", [tmp_path]) is None + + +def test_returns_none_for_empty_search_paths(): + assert find_first_package_dir("MyPackage", None) is None + + +def test_find_package_dir_still_rejects_ambiguity(two_dirs): + """The all-at-once helper keeps its previous behaviour.""" + work_dir, extra_dir = two_dirs + + with pytest.raises(ValueError): + find_package_dir("MyPackage", [work_dir, extra_dir]) + + +@pytest.fixture +def two_script_dirs(tmp_path): + """A script dir and an additional dir, both holding 'MyPackage.py'.""" + script_dir = tmp_path / "scripts" + extra_dir = tmp_path / "extra_scripts" + script_dir.mkdir() + extra_dir.mkdir() + (script_dir / "MyPackage.py").write_text("# working copy") + (extra_dir / "MyPackage.py").write_text("# dependency copy") + return script_dir, extra_dir + + +def test_prefers_the_first_search_path_for_scripts(two_script_dirs): + """The script lookup takes the same route, so files must resolve too.""" + script_dir, extra_dir = two_script_dirs + + found = find_first_package_dir("MyPackage.py", [script_dir, extra_dir]) + + assert found == script_dir / "MyPackage.py" + + +def test_falls_through_to_a_later_script_dir(two_script_dirs, tmp_path): + _, extra_dir = two_script_dirs + empty_dir = tmp_path / "empty_scripts" + empty_dir.mkdir() + + found = find_first_package_dir("MyPackage.py", [empty_dir, extra_dir]) + + assert found == extra_dir / "MyPackage.py"