Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 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
72 changes: 70 additions & 2 deletions 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 All @@ -39,7 +39,7 @@ def _get_examples_dir() -> Path:
FileNotFoundError
If the examples directory cannot be located in the installation.
"""
with get_package_dir() as pkgdir:
with get_package_dir(root_path) as pkgdir:
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this (yet). Here the function is being called, not defined, and we are still working on the API and tests.

pkg = Path(pkgdir).resolve()
for c in (
pkg / "docs" / "examples",
Expand Down Expand Up @@ -72,6 +72,74 @@ def map_pack_to_examples() -> dict[str, List[str]]:
return examples_by_pack


def copy_examples(
examples: str | 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 : str or list of str
Example name(s): 'example1' or ['example1', 'example2']
target_dir : Path, optional
Target directory where examples should be copied.
Defaults to current working directory if not specified.
overwrite : bool, default False
If True, overwrite existing directories. If False, raise
FileExistsError when destination exists.

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 copy_packs(
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we copying packs? I am not sure what this means. Don't we want to copy examples?

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 meant this as it copies examples given paths, but since were using a dictionary to hold everything this is unnecessary. We can just have one function copy_examples

packs: str | List[str],
target_dir: Optional[Path] = None,
) -> List[Path]:
"""Copy all examples from one or more packs to a target directory.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about this and the above function? This could be broken out into 4 functions for each UC (copy one ex, one pack, multiple exs, or multiple packs). Or, it could be combined into one function that handles all 4 of these use cases. Right now, its at the middle ground with two functions which felt like a safe in-between place.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this function because we have (or will have) a copy_examples function which copies examples, right? I think we can handle all the copy UCs in one function, no problem.

Also, we may want to decide if we want this to be a method in the PacksManager class or standalone here. It can probably be argued both ways, and we can look in main to see how these are handled there by Tieqiong,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it stands, copy_examples is not a method in PacksManager. I think its okay to leave it as a standalone function.


Parameters
----------
packs : str or list of str
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want this structure? Maybe better to just take a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, true. just take a list

Pack name(s). Can be:
- Single pack name: 'pack1'
- List of pack names: ['pack1', 'pack2']
- Special keyword: 'all' (copies all packs)
target_dir : Path, optional
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.

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.

why do we return this?

List of destination paths created (all examples from the pack(s)).

Raises
------
ValueError
If pack name is invalid.
FileNotFoundError
If pack does not exist.
FileExistsError
If any destination exists.
"""
return


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

Expand Down
9 changes: 9 additions & 0 deletions src/diffpy/cmi/packsmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ def available_packs(self) -> List[str]:
p.stem for p in self.packs_dir.glob("*.txt") if p.is_file()
)

def available_examples(self):
"""Finds all examples for each pack and builds a dict."""
return

def print_info(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs to an input which contains packs and example file names, and possibly a path.

Like the copy function, this could either be in cli.py or here. But if this is here, then so should the copy function be here as the yare basically doing similar tasks (generating output behavior in response to a user input)

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 think I like this function in cli.py. Then we can use the available_examples method to generate the packs and examples dict

"""Pretty print available and installed packs and examples to
console."""
return

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

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
41 changes: 41 additions & 0 deletions tests/test_packsmanager.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import pytest

from diffpy.cmi.packsmanager import PacksManager


@pytest.mark.parametrize(
"expected_dict",
[
{
"pdf": [
"ch03NiModelling",
"ch06RefineCrystalStructureGen",
"ch07StructuralPhaseTransition",
"ch08NPRefinement",
]
}
],
)
def test_available_examples(expected_dict):
"""Test that available_examples returns a dict."""
Copy link
Contributor

Choose a reason for hiding this comment

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

no docstring, but please write all the cases you want to test, then we can write the code to do that.

pkmg = PacksManager()
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be instantiated with a tmp_dir filepath and use paramatrize for difference cases

returned_dict = pkmg.available_examples()
expected_pack = list(expected_dict.keys())
returned_pack = list(returned_dict.keys())
for pack in expected_pack:
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 too specific I think because some cases won't have packs, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

assert pack in returned_pack, f"{pack} not found in returned packs."
expected_examples = expected_dict[pack]
Copy link
Contributor

Choose a reason for hiding this comment

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

call this expected

returned_examples = returned_dict.get(pack, [])
Copy link
Contributor

Choose a reason for hiding this comment

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

we normally would call this actual

for ex in expected_examples:
assert (
ex in returned_examples
), f"{ex} not found under pack {pack}."


def test_print_info(capsys):
"""Test that print_info prints expected information to stdout."""
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

pkmg = PacksManager()
pkmg.print_info()
captured = capsys.readouterr()
output = captured.out.strip()
assert "Available packs" in output or "Installed packs" in output
Loading