Skip to content
Closed
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
54974ba
initial listing functions for cli
cadenmyers13 Oct 6, 2025
c9cf238
initial tests
cadenmyers13 Oct 6, 2025
f9a14c3
test for listing available examples
cadenmyers13 Oct 6, 2025
71423d5
test pretty print
cadenmyers13 Oct 6, 2025
a1ee907
api for copying packs and examples
cadenmyers13 Oct 6, 2025
b16c90b
have one function for copying examples
cadenmyers13 Oct 7, 2025
15c8111
move print_info to cli, add params to docstring in available_examples
cadenmyers13 Oct 7, 2025
db2ac9a
add fnferror to api
cadenmyers13 Oct 7, 2025
9fcef71
update available_examples test
cadenmyers13 Oct 7, 2025
c70ca27
have pkmg methods take temp_path instead of pkmg
cadenmyers13 Oct 7, 2025
d32201a
test print info and test copy examples
cadenmyers13 Oct 7, 2025
fbbf88c
checkpoint1 commit of working available examples tests
cadenmyers13 Oct 7, 2025
a458c5e
checkpoint conftest, build example_cases fixture
cadenmyers13 Oct 7, 2025
736b519
checkpoint packsmanager.py file
cadenmyers13 Oct 8, 2025
0bd26fc
checkpoint: conftest is building correctly
cadenmyers13 Oct 8, 2025
4be38f6
checkpoint, working version that gets examples dir outside of PacksMa…
cadenmyers13 Oct 8, 2025
280390c
checkpoint: _get_examples_dir moved back in PacksManager and paths ar…
cadenmyers13 Oct 8, 2025
99c4951
forgot to add conftest to previous checkpoint commit, so its added in…
cadenmyers13 Oct 8, 2025
aa65d02
checkpoint: test that the path and examples match
cadenmyers13 Oct 8, 2025
4aa15b0
checkpoint: add all 4 cases to test_packsmanager
cadenmyers13 Oct 8, 2025
cfb2273
Add case 5, split into separate tests, and refactor code for style/re…
cadenmyers13 Oct 9, 2025
fdf09b2
sort packs and examples when building dict with available_examples()
cadenmyers13 Oct 9, 2025
53ac9a2
add case 5 to conftest
cadenmyers13 Oct 9, 2025
ce1efa7
remove printed fail statement
cadenmyers13 Oct 9, 2025
c39dfae
rm unused helper function I made in conftest
cadenmyers13 Oct 9, 2025
c2e8076
add comment for case5 ex
cadenmyers13 Oct 9, 2025
d1038b7
UCs for copy_examples tests
cadenmyers13 Oct 9, 2025
85bb747
treat each 'caseX' as the root dir, build out from there
cadenmyers13 Oct 9, 2025
0d08dbe
change wording for comment
cadenmyers13 Oct 9, 2025
fcbb1a2
update innit to accept root_path
cadenmyers13 Oct 9, 2025
52aa4e7
better function name
cadenmyers13 Oct 9, 2025
e8c8ee5
revert some files to upstream/main, doing only dict building tests on…
cadenmyers13 Oct 10, 2025
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
14 changes: 13 additions & 1 deletion src/diffpy/cmi/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,19 @@
from importlib.resources import as_file, files


def get_package_dir():
def get_package_dir(root_path=None):
"""Get the package directory as a context manager.

Parameters
----------
root_path : str, optional
Used for testing, overrides the files(__name__) call.

Returns
-------
context manager
A context manager that yields a pathlib.Path to the package directory.
"""
resource = files(__name__)
return as_file(resource)

Expand Down
39 changes: 38 additions & 1 deletion src/diffpy/cmi/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@


# Examples
def _get_examples_dir() -> Path:
def _get_examples_dir(root_path=None) -> Path:
"""Return the absolute path to the installed examples directory.

Returns
Expand Down Expand Up @@ -72,6 +72,43 @@ def map_pack_to_examples() -> dict[str, List[str]]:
return examples_by_pack


def copy_examples(
examples: List[str],
target_dir: Optional[Path] = None,
) -> List[Path]:
"""Copy one or more examples to a target directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

how about:
"""Copy one or more examples from the installed package to a target directory.


Parameters
----------
examples : list of str
Example name(s): ['example1'], ['pack1']
target_dir : Path, optional
Target directory where examples should be copied.
Defaults to current working directory if not specified.

Returns
-------
list of Path
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we need to return this. How will it be used?

List of destination paths created.

Raises
------
ValueError
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to raise this. We just copy all versions that we find. This will be a case we test.

We often don't do this Raises section, the interesting raises are caught in the tests. Maybe we should but our current style is not to. It can be tmi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I'll leave it empty for now

If example name is ambiguous (exists in multiple packs).
FileNotFoundError
If example does not exist.
FileExistsError
If destination exists and overwrite=False.
"""
return


def print_info(pack_examples_dict):
"""Pretty print available and installed packs and examples to
console."""
return


def copy_example(pack_example: str) -> Path:
"""Copy an example into the current working directory.

Expand Down
61 changes: 58 additions & 3 deletions src/diffpy/cmi/packsmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
__all__ = ["PacksManager"]


def _installed_packs_dir() -> Path:
def _installed_packs_dir(root_path=None) -> Path:
"""Locate requirements/packs/ for the installed package."""
with get_package_dir() as pkgdir:
pkg = Path(pkgdir).resolve()
Expand All @@ -51,10 +51,34 @@ class PacksManager:
packs_dir : pathlib.Path
Absolute path to the installed packs directory.
Defaults to `requirements/packs` under the installed package.
examples_dir : pathlib.Path
Absolute path to the installed examples directory.
Defaults to `docs/examples` under the installed package.
"""

def __init__(self) -> None:
self.packs_dir = _installed_packs_dir()
def __init__(self, root_path=None) -> None:
if root_path is None:
self.packs_dir = _installed_packs_dir(root_path)
self.examples_dir = self._get_examples_dir()
# root_path option provided for testing
else:
self.packs_dir = Path(root_path).resolve()
self.examples_dir = Path(root_path).resolve()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be easier to read if we just move self.examples_dir = self._get_examples_dir() outside of the if/else clause.


def _get_examples_dir(self) -> Path:
"""Return the absolute path to the installed examples directory.

Returns
-------
pathlib.Path
Directory containing shipped examples.

Raises
------
FileNotFoundError
If the examples directory cannot be located in the installation.
"""
return (self.packs_dir / ".." / ".." / "docs" / "examples").resolve()

def available_packs(self) -> List[str]:
"""List all available packs.
Expand All @@ -68,6 +92,37 @@ def available_packs(self) -> List[str]:
p.stem for p in self.packs_dir.glob("*.txt") if p.is_file()
)

def available_examples(self) -> dict[str, List[tuple[str, Path]]]:
"""Finds all examples for each pack and builds a dict.

Parameters
----------
root_path : Path
Root path to the examples directory.
Returns
-------
dict
A dictionary mapping pack names to lists of example names.

Raises
------
FileNotFoundError
If the provided root_path does not exist or is not a directory.
"""
example_dir = self.examples_dir
examples_dict = {}
for pack_path in sorted(example_dir.iterdir()):
if pack_path.is_dir():
pack_name = pack_path.stem
examples_dict[pack_name] = []
for example_path in sorted(pack_path.iterdir()):
if example_path.is_dir():
example_name = example_path.stem
examples_dict[pack_name].append(
(example_name, example_path)
)
return examples_dict

def _resolve_pack_file(self, identifier: Union[str, Path]) -> Path:
"""Resolve a pack identifier to an absolute .txt path.

Expand Down
72 changes: 72 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,78 @@ def tmp_examples(tmp_path_factory):
yield tmp_examples


@pytest.fixture(scope="session")
def example_cases(tmp_path_factory):
"""Copy the entire examples/ tree into a temp directory once per
test session.

Returns the path to that copy.
"""
examples_dir = tmp_path_factory.mktemp("examples")

# case 1: pack with no examples
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need a requirements/packs dir that we we will use as root_dir in the test

Copy link
Contributor Author

@cadenmyers13 cadenmyers13 Oct 9, 2025

Choose a reason for hiding this comment

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

See the tree structure here: #53 (comment). This is what the temp dir looks like. let me know if this looks okay. I believe everything is being correctly and there is no magic

# case1_dict = {"case1": {"empty_pack": []}}
# _build_examples_tree_helper(examples_dir, case1_dict)
case1 = examples_dir / "case1" / "empty_pack"
case1.mkdir(parents=True, exist_ok=True)

# Case 2: pack with multiple examples
case2a = (
examples_dir
/ "case2"
/ "full_pack"
/ "example1"
/ "solution"
/ "diffpy-cmi"
) # full_pack, example1
case2a.mkdir(parents=True, exist_ok=True)
(case2a / "script1.py").touch()
case2b = (
examples_dir / "case2" / "full_pack" / "example2" / "random" / "path"
) # full_pack, example2
case2b.mkdir(parents=True, exist_ok=True)
(case2b / "script1.py").touch()
(case2b / "script2.py").touch()

# # Case 3: multiple packs with multiple examples
case3a = examples_dir / "case3" / "packA" / "my_ex1" # packA, ex1
case3a.mkdir(parents=True, exist_ok=True)
(case3a / "script1.py").touch()
case3b = (
examples_dir / "case3" / "packA" / "my_ex2" / "solutions"
) # packA, ex2
case3b.mkdir(parents=True, exist_ok=True)
(case3b / "script2.py").touch()
case3c = (
examples_dir / "case3" / "packB" / "ex1" / "more" / "random" / "path"
) # packB, ex1
case3c.mkdir(parents=True, exist_ok=True)
(case3c / "script3.py").touch()
(case3c / "script4.py").touch()

# # Case 4: no pack found (empty directory)
case4 = examples_dir / "case4"
case4.mkdir(parents=True, exist_ok=True)

# Case 5: multiple packs with the same example names
case5a = examples_dir / "case5" / "packA" / "example" / "path1"
case5a.mkdir(parents=True, exist_ok=True)
(case5a / "script1.py").touch()
case5b = examples_dir / "case5" / "packB" / "example" / "path2"
case5b.mkdir(parents=True, exist_ok=True)
(case5b / "script2.py").touch()

yield examples_dir


@pytest.fixture(scope="session")
def copy_target_dir(tmp_path_factory):
"""Create a temporary directory to serve as the target for copying
examples."""
target_dir = tmp_path_factory.mktemp("copy_target")
yield target_dir


@pytest.fixture(scope="session", autouse=True)
def use_headless_matplotlib():
"""Force matplotlib to use a headless backend during tests."""
Expand Down
11 changes: 11 additions & 0 deletions tests/test__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# from diffpy.cmi import get_package_dir
from pathlib import Path

import pytest


@pytest.mark.parametrize("root_path", [None, str(Path(__file__).parent)])
def test_get_package_dir(root_path):
"""Test that get_package_dir returns a valid path context
manager."""
assert False
18 changes: 18 additions & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,24 @@
import pytest

from diffpy.cmi import cli
from diffpy.cmi.packsmanager import PacksManager


def test_print_info(temp_path, capsys):
Copy link
Contributor

Choose a reason for hiding this comment

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

heads up, I am, not reviewing these till we have the dict building done.

pkmg = PacksManager()
actual = pkmg.available_examples(temp_path)
# pretty print the actual dict
pkmg.print_info(actual)
captured = capsys.readouterr()
output = captured.out.strip()
# check that output contains expected headers
assert "Available packs" in output or "Installed packs" in output


@pytest.mark.parametrize()
def test_copy_examples(dict, tmp_path):
cli.copy_examples(examples=dict, target_dir=tmp_path)
assert False


def test_map_pack_to_examples_structure():
Expand Down
87 changes: 87 additions & 0 deletions tests/test_packsmanager.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import pytest

from diffpy.cmi.packsmanager import PacksManager

example_params = [
# 1) pack with no examples. Expect {'empty_pack': []}
# 2) pack with multiple examples.
# Expect {'full_pack': [('example1`, path_to_1'), 'example2', path_to_2)]
# 3) multiple packs. Expect dict with multiple pack:tuple pairs
# 4) no pack found. Expect {}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we leave these here, we need to add case 5 here also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

# case 1: pack with no examples. Expect {'empty_pack': []}
(
"case1",
{"empty_pack": []},
),
# case 2: pack with multiple examples.
# Expect {'full_pack': [('example1', path_to_1),
# ('example2', path_to_2)]}
(
"case2",
{
"full_pack": [
("example1", "case2/full_pack/example1"),
("example2", "case2/full_pack/example2"),
]
},
),
# case 3: multiple packs. Expect dict with multiple pack:tuple pairs
(
"case3",
{
"packA": [
("my_ex1", "case3/packA/my_ex1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

not a big deal, but to make the intent cleearer let's call the examples exampleA1 and so on and use the same pattern for all the cases.

("my_ex2", "case3/packA/my_ex2"),
],
"packB": [("ex1", "case3/packB/ex1")],
},
),
( # case 4: no pack found. Expect {}
"case4",
{},
),
( # case 5: multiple packs with the same example names
Copy link
Contributor

Choose a reason for hiding this comment

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

good, nicely done there! maybe even duplicate_example to be super explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

# Expect dict with multiple pack:tuple pairs
"case5",
{
"packA": [
("example", "case5/packA/example"),
],
"packB": [
("example", "case5/packB/example"),
],
},
),
]


@pytest.mark.parametrize("input,expected", example_params)
def test_available_examples(input, expected, example_cases):
tmp_ex_dir = example_cases / input
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, looking at this, we will need a requirements/packs for each case..... but let's do it anyway to make things clearer and less brittle. We could even add requirements files in there for when we write tests for the pack installation code....

pkmg = PacksManager(tmp_ex_dir)
# Ensure the example directory is correctly set
assert pkmg.examples_dir == tmp_ex_dir
Copy link
Contributor

Choose a reason for hiding this comment

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

this assert is incorrect. tmp_ex_dir should point to a requirements/packs directory (we should probably give it a different name too)

actual = pkmg.available_examples()
# Verify that the keys (pack names) are correct
Copy link
Contributor

Choose a reason for hiding this comment

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

please delete gratuitous comments

assert set(actual.keys()) == set(expected.keys())
# Verify that each expected example exists in actual
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is also gratuitous, but a more informational comment would be helpful here. Something like # unpack the actual dict to test the values individually. global identity will fail because of the temp path

for expected_pack, expected_list in expected.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

I asked chatGPT for a more pythonic way of doing this and got the following, which I think I prefer:

def values_match(list_a, list_b, tol=1e-9):
    if len(list_a) != len(list_b):
        return False
    for (num_a, str_a), (num_b, str_b) in zip(list_a, list_b):
        if abs(num_a - num_b) > tol:
            return False
        if not (str_a in str_b or str_b in str_a):
            return False
    return True

all(values_match(a[k], b[k]) for k in a)

function definition would be outside the test function, which makes the test function very readable. Of course variable names etc. should be modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented something similar

actual_list = actual[expected_pack]
for (expected_exname, expected_path), (
actual_exname,
actual_path,
) in zip(expected_list, actual_list):
# Checks example name and path
assert expected_exname == actual_exname
assert expected_path == str(actual_path.relative_to(example_cases))


@pytest.mark.parametrize("input,expected", example_params)
def test_tmp_(input, expected, example_cases):
Copy link
Contributor

Choose a reason for hiding this comment

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

this is good

example_path = example_cases / input
for path in example_path.rglob("*"):
if path.suffix:
# Checks temp files are files and not dirs
Copy link
Contributor

Choose a reason for hiding this comment

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

pls remove

assert path.is_file()
else:
assert path.is_dir()
Loading