Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0f3293f
move examples under pdf dir
cadenmyers13 Sep 23, 2025
c5c9f84
edit cli to reflect new path change
cadenmyers13 Sep 23, 2025
1092dcb
news
cadenmyers13 Sep 23, 2025
14102ad
fix typo
cadenmyers13 Sep 23, 2025
d423377
Revise examples in news/ex-path.rst for clarity
sbillinge Sep 24, 2025
683b4c3
rm type hint
cadenmyers13 Sep 25, 2025
71f9de4
merge to local: move news from added to changed
cadenmyers13 Sep 25, 2025
3a1ee8b
change list_examples to map_pack_to_examples which returns a dict
cadenmyers13 Sep 25, 2025
d3b531c
change variable name to reflect action properly
cadenmyers13 Sep 25, 2025
73d083e
change function name
cadenmyers13 Sep 25, 2025
61a9bd6
update docstring
cadenmyers13 Sep 25, 2025
012a45e
Merge branch 'main' of github.com:diffpy/diffpy.cmi into ex-path
cadenmyers13 Sep 25, 2025
d90a271
tests for help command, listing examples, and copying examples
cadenmyers13 Sep 25, 2025
c12f6b5
fix monkeypatch debugging code
cadenmyers13 Sep 25, 2025
c0bead8
update function names to be more readable
cadenmyers13 Sep 26, 2025
63ac630
test map pack to examples function
cadenmyers13 Sep 26, 2025
e0a2c75
add no packs, no examples case
cadenmyers13 Sep 26, 2025
81fa5cf
make sure directory was created
cadenmyers13 Sep 26, 2025
10771b1
add tests for copy_example
cadenmyers13 Sep 26, 2025
4ee36af
Add Error if pack or example doesnt exist
cadenmyers13 Sep 26, 2025
4019e36
Merge branch 'main' ex-path to see core example
cadenmyers13 Sep 26, 2025
65fd2c7
map packs function test, simplified
cadenmyers13 Sep 29, 2025
74e992b
add condition that catches extra slashes
cadenmyers13 Sep 29, 2025
d53fab8
test copy behavior
cadenmyers13 Sep 29, 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
23 changes: 23 additions & 0 deletions news/ex-path.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* <news item>

**Changed:**

* change examples directory structure to insert the name of the ``pack" that the examples exemplify.

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
48 changes: 31 additions & 17 deletions src/diffpy/cmi/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@


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

Returns
Expand All @@ -52,27 +52,33 @@ def _installed_examples_dir() -> Path:
)


def list_examples() -> List[str]:
"""List installed example names.
def map_pack_to_examples() -> dict[str, List[str]]:
"""Return a dictionary mapping pack name -> list of example
subdirectories.

Returns
-------
list of str
Installed example directory names.
dict:
pack name -> list of example subdirectory names
"""
root = _installed_examples_dir()
root = _get_examples_dir()
if not root.exists():
return []
return sorted([p.name for p in root.iterdir() if p.is_dir()])
return {}
examples_by_pack = {}
for pack_dir in sorted(root.iterdir()):
if pack_dir.is_dir():
exdirs = sorted(p.name for p in pack_dir.iterdir() if p.is_dir())
examples_by_pack[pack_dir.name] = exdirs
return examples_by_pack
Copy link
Contributor Author

@cadenmyers13 cadenmyers13 Sep 25, 2025

Choose a reason for hiding this comment

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

returns a dict, mapping pack name (str) to its corresponding list of examples (list of strs). This replaces previous format were all the examples are stored in one list.



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

Parameters
----------
example : str
Example directory name under the installed examples root.
Pack and example name in the form ``<pack>/<exdir>``.

Returns
-------
Expand All @@ -81,15 +87,20 @@ def copy_example(example: str) -> Path:

Raises
------
ValueError
If the format is invalid.
FileNotFoundError
If the example directory does not exist.
FileExistsError
If the destination directory already exists.
"""
src = _installed_examples_dir() / example
if "/" not in pack_example:
raise ValueError("Example must be specified as <pack>/<exdir>")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enforces new format of copying examples. This format is cmi example copy <pack>/<exdir>

pack, exdir = pack_example.split("/", 1)
src = _get_examples_dir() / pack / exdir
if not src.exists() or not src.is_dir():
raise FileNotFoundError(f"Example not found: {example}")
dest = Path.cwd() / example
raise FileNotFoundError(f"Example not found: {pack_example}")
dest = Path.cwd() / exdir
if dest.exists():
raise FileExistsError(f"Destination {dest} already exists")
copytree(src, dest)
Expand Down Expand Up @@ -163,7 +174,9 @@ def _build_parser() -> argparse.ArgumentParser:
_parser=p_example
)
p_example_copy = sub_ex.add_parser("copy", help="Copy an example to CWD")
p_example_copy.add_argument("name", metavar="EXAMPLE", help="Example name")
p_example_copy.add_argument(
"name", metavar="EXAMPLE", help="Example name <pack>/<exdir>"
)
p_example_copy.set_defaults(_parser=p_example)
p_example.set_defaults(example_cmd=None)

Expand Down Expand Up @@ -339,10 +352,11 @@ def _cmd_example(ns: argparse.Namespace) -> int:
print(f"Example copied to: {out}")
return 0
if ns.example_cmd == "list":
for g in list_examples():
print(g)
for pack, examples in map_pack_to_examples().items():
print(f"{pack}:")
for ex in examples:
print(f" - {ex}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prints examples in the format of

$ cmi example list
pack1:
 - exampleA
 - exampleB
pack2:
 - exampleC
 - exampleD

instead of

$ cmi example list
 exampleA
 exampleB
 exampleC
 exampleD

return 0

plog.error("Unknown example subcommand.")
ns._parser.print_help()
return 2
Expand Down
40 changes: 40 additions & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import pytest

from diffpy.cmi import cli


@pytest.mark.parametrize(
"structure, expected",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change structure to inputs or sthg like that?

[
# case: no packs, no examples
([], {}),
# case: one pack with one example
Copy link
Contributor

Choose a reason for hiding this comment

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

much much better. Even better would be

        # case: one pack with one example. expect dict of "{pack-name: [example]}"

arguably I can see that from the code below, but it is easier and quicker for the reviewer if the intent is right there.

([("packA", ["ex1"])], {"packA": ["ex1"]}),
# case: one pack with multiple examples
([("packA", ["ex1", "ex2"])], {"packA": ["ex1", "ex2"]}),
# case: multiple packs with one example each
(
[("packA", ["ex1"]), ("packB", ["ex2"])],
{"packA": ["ex1"], "packB": ["ex2"]},
),
# case: multiple packs with multiple examples
(
[("packA", ["ex1", "ex2"]), ("packB", ["ex3", "ex4"])],
{"packA": ["ex1", "ex2"], "packB": ["ex3", "ex4"]},
),
],
)
def test_map_pack_to_examples(tmp_path, mocker, structure, expected):
"""Finds examples directory and returns a dictionary mapping packs
to examples."""
# example input: build example structure
Copy link
Contributor

Choose a reason for hiding this comment

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

remove gratuitous unneeded comments. Rather, write the code in a way that it is directly readable. Comments can be added if the code is not clear for some reason that can't be changed.

for pack, exdirs in structure:
packdir = tmp_path / pack
packdir.mkdir()
for ex in exdirs:
(packdir / ex).mkdir()
# patch _get_examples_dir to point to tmp_path
mocker.patch.object(cli, "_get_examples_dir", return_value=tmp_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

only mock things in exceptional circumstances. If you have to mock it could mean you have a bad code design because things are not well separated. Here it is just not needed, not necessarily bad design.

Here we want to build examples directories in tmpdir and then have the function go get them, so the test will look like

examples_path = logic that builds the examples from the inputs
actual = build_examples_doc(examples_path)
assert actual == expected

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 think the examples may be used by more than one test build them in conftest.py. We could use a structure like:

tmpdir / example-case-1 / docs / examples / pack_a / example_a / solutions / diffpy-cmi / solution1.py
            / example-case-2 / docs / examples / pack_a / example_a / solutions / diffpy-cmi / solution1.py
                                                                                     / example_b / solutions / diffpy-cmi / solution1.py

and so on. The function would have to file_path as an input

Copy link
Contributor Author

@cadenmyers13 cadenmyers13 Sep 26, 2025

Choose a reason for hiding this comment

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

only mock things in exceptional circumstances. If you have to mock it could mean you have a bad code design because things are not well separated. Here it is just not needed, not necessarily bad design.

@sbillinge _get_examples_dir() always returns the actual path to the examples and not the temp dir path (it returns diffpy.cmi/docs/examples). Since map_pack_to_examples() calls it, I mocked it so that the output path is the temp path rather than the actual path. I don't see a way of testing the behavior unless we change the function? Here's the function for reference:

def _get_examples_dir() -> 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.
    """
    with get_package_dir() as pkgdir:
        pkg = Path(pkgdir).resolve()
        for c in (
            pkg / "docs" / "examples",
            pkg.parents[2] / "docs" / "examples",
        ):
            if c.is_dir():
                return c
    raise FileNotFoundError(
        "Could not locate requirements/packs. Check your installation."
    )
    ```

Copy link
Contributor Author

@cadenmyers13 cadenmyers13 Sep 26, 2025

Choose a reason for hiding this comment

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

We could add an override feature to this function but that seems pretty much identical to mocking. plus is very ugly

Copy link
Contributor

Choose a reason for hiding this comment

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

Si the problem here may be a misunderstanding of how tests work. You seen to be writing the test to test the function, but we want to do it the other way around. We want to write the function after we write the test. So we write the test to capture the behavior we want.

If I were thinking about this I might say, "oh, I want this function to iterate over a file structure and collect all the examples it finds and load them in a dict that it returns. In that case I would expect it in general to take a path as input and return the dict. This allows the user to reuse the function in different ways. If the user is the test, it can then specify the tmpdir.

The tests then help to guide us to a better design.

# expected behavior: a dictionary mapping pack to lists of examples
result = cli.map_pack_to_examples()
assert result == expected
Loading