feat: build examples dict for cli commands #53

cadenmyers13 · 2025-10-06T19:26:27Z

cli commands

github-actions · 2025-10-06T19:26:50Z

Warning! No news item is found for this PR. If this is a user-facing
change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.
For best practices, please visit
https://scikit-package.github.io/scikit-package/frequently-asked-questions.html#billinge-group-standards.

cadenmyers13 · 2025-10-06T20:15:39Z

@sbillinge test test_available_examples ready for review. Also if you have any comments on my API that would be great. those are still a work in progress.

cadenmyers13 · 2025-10-06T21:02:08Z

pretty print test function is also ready for review

src/diffpy/cmi/cli.py

cadenmyers13 · 2025-10-06T22:34:53Z

src/diffpy/cmi/cli.py

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


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.

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,

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

src/diffpy/cmi/cli.py

sbillinge · 2025-10-07T00:41:23Z

src/diffpy/cmi/cli.py

        If the examples directory cannot be located in the installation.
    """
-    with get_package_dir() as pkgdir:
+    with get_package_dir(root_path) as pkgdir:


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

sbillinge · 2025-10-07T00:42:52Z

src/diffpy/cmi/cli.py

+
+    Parameters
+    ----------
+    packs : str or list of str


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

yeah, true. just take a list

sbillinge · 2025-10-07T00:43:39Z

src/diffpy/cmi/cli.py

+    return
+
+
+def copy_packs(


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

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

sbillinge · 2025-10-07T01:01:59Z

tests/test_packsmanager.py

+    for pack in expected_pack:
+        assert pack in returned_pack, f"{pack} not found in returned packs."
+        expected_examples = expected_dict[pack]
+        returned_examples = returned_dict.get(pack, [])


we normally would call this actual

sbillinge · 2025-10-07T01:02:19Z

tests/test_packsmanager.py

+    returned_pack = list(returned_dict.keys())
+    for pack in expected_pack:
+        assert pack in returned_pack, f"{pack} not found in returned packs."
+        expected_examples = expected_dict[pack]


call this expected

sbillinge · 2025-10-07T01:03:31Z

tests/test_packsmanager.py

+    returned_dict = pkmg.available_examples()
+    expected_pack = list(expected_dict.keys())
+    returned_pack = list(returned_dict.keys())
+    for pack in expected_pack:


this is too specific I think because some cases won't have packs, right?

sbillinge · 2025-10-07T01:03:51Z

tests/test_packsmanager.py

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


sbillinge · 2025-10-07T01:03:54Z

tests/test_packsmanager.py

+    ],
+)
+def test_available_examples(expected_dict):
+    """Test that available_examples returns a dict."""


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

cadenmyers13 · 2025-10-07T03:31:42Z

@sbillinge responded to comments and made changes. still need to address comments on test__init__.py though

tests/test_packsmanager.py

sbillinge

good progress. Please see my comments.

src/diffpy/cmi/cli.py

sbillinge · 2025-10-07T11:36:32Z

src/diffpy/cmi/cli.py

+
+    Returns
+    -------
+    list of Path


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

sbillinge · 2025-10-07T11:39:06Z

src/diffpy/cmi/cli.py

+
+    Raises
+    ------
+    ValueError


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

Okay I'll leave it empty for now

sbillinge · 2025-10-07T11:40:06Z

src/diffpy/cmi/cli.py

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


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

src/diffpy/cmi/packsmanager.py

sbillinge · 2025-10-07T11:48:57Z

tests/test_packsmanager.py

+    "expected",
+    [
+        {
+            # test with pack that has examples


How about this?

# pack with no examples. Expect {'empty_pack': []} # pack with multiple examples. Expect {'full_pack': [('example1`, path_to_1'), 'example2', path_to_2)] # multiple packs. Expect dict with multiple pack:tuple pairs # no pack found. Expect {}

sbillinge · 2025-10-07T11:52:21Z

tests/test_packsmanager.py

+    ],
+)
+def test_available_examples(temp_path, expected):
+    for pack, examples in expected.items():


This is all wrong, but we can revisit this when we have the cases sorted out. Basically, we will need to build a different set of folders for each case and the paramertrize will have to hand the path to each case as an input. Paramatrize usually has a kind of [('input','expected')] feel to it which it doesn't atm here.

sbillinge · 2025-10-07T11:53:20Z

tests/test_packsmanager.py

+            ex_dir = pack_dir / ex
+            ex_dir.mkdir(parents=True, exist_ok=True)
+    pkmg = PacksManager(temp_path)
+    actual = pkmg.available_examples(temp_path)


you seem to be handing temp_path to this twice. Think about how you want it to work (the behavior you want) and then do it that way.

I've made the change. the methods are taking temp_path as input

sbillinge · 2025-10-07T12:21:23Z

we need a news

…nager

…e being found. the keys of actual and expected are asserting True

… this commit

cadenmyers13 · 2025-10-08T18:30:53Z

@sbillinge I was able to move _get_examples_dir back inside PacksManager. Let me know what you think of the current packsmanager.py and its test.

cadenmyers13 · 2025-10-08T18:58:33Z

Also added test for the stored examples and packs. There are some sporadic comments that can be ignored for now until we have a complete version

sbillinge

great progress. Please see comments. Mostly on style.

tests/test_packsmanager.py

sbillinge · 2025-10-08T19:25:02Z

tests/test_packsmanager.py

+    ],
+)
+def test_available_examples(input, expected, example_cases):
+    root_path = example_cases / input


give this a more readable name than root_path Give it a name that makes logical sense.

called it tmp_ex_dir since it terminates at examples/caseX. This also pairs well with the assert case below

sbillinge · 2025-10-08T19:25:24Z

tests/test_packsmanager.py

+def test_available_examples(input, expected, example_cases):
+    root_path = example_cases / input
+    pkmg = PacksManager(root_path)
+    print()


remove all debugging print statements and comments.

In general, replace with a comment like the cases above about what behavior is being tested. But see below.

sbillinge · 2025-10-08T19:25:36Z

tests/test_packsmanager.py

+    # print("packsmananger_dir:", pkmg.examples_dir)
+    # print("root_path:", root_path)
+    for path in root_path.rglob("*"):
+        print(" -", path.relative_to(example_cases))


remove debugging prints

sbillinge · 2025-10-08T19:28:03Z

tests/test_packsmanager.py

+    print()
+    # print("packsmananger_dir:", pkmg.examples_dir)
+    # print("root_path:", root_path)
+    for path in root_path.rglob("*"):


move this out of this test. I like that we have these tests, but they should be run in a separate function, something like test_tmp_filesystem() or sthg, so they are just run once.

sbillinge · 2025-10-08T19:28:46Z

tests/test_packsmanager.py

+    # print(" +", path.relative_to(pkmg.examples_dir.parent))
+
+    actual = pkmg.available_examples()
+    print("expected:")


remove remove remove etc.

sbillinge · 2025-10-08T19:29:21Z

tests/test_packsmanager.py

+
+    assert (
+        actual.keys() == expected.keys()
+    ), f"Keys differ: expected {expected.keys()}, got {actual.keys()}"


we don't need printed outputs for test fails.

sbillinge · 2025-10-08T19:29:48Z

tests/test_packsmanager.py

+    assert (
+        actual.keys() == expected.keys()
+    ), f"Keys differ: expected {expected.keys()}, got {actual.keys()}"
+    # if values of excepted are in actual assert true


ummm why is this here?

no reason, I didn't clean comments before pushing. its been removed

sbillinge · 2025-10-08T19:30:55Z

tests/test_packsmanager.py

+    # if values of excepted are in actual assert true
+    # assert actual == expected
+    for expected_pack, expected_tuple in expected.items():
+        assert expected_pack in actual, f"Missing pack: {expected_pack}"


this assert could be stricter... ==

remove comment

On second thought, we can probably just remove this assert because about with have

assert actual.keys() == expected.keys()

which does the same thing.

sbillinge · 2025-10-08T19:32:26Z

tests/test_packsmanager.py

+    for expected_pack, expected_tuple in expected.items():
+        assert expected_pack in actual, f"Missing pack: {expected_pack}"
+        for expected_name, expected_rel_path in expected_tuple:
+            matches = [


this seems to be a rather unintuitive and unreadable way of doing this, is there a reason you chose this approach?

I agree its unreadable. I've made the change to make it more readable.

…adability

cadenmyers13 · 2025-10-09T15:22:31Z

@sbillinge I made a push to this branch addressing your comments but for some reason its not showing up here. Its showing up on my origin though... https://github.com/cadenmyers13/diffpy.cmi/blob/making-cli/tests/test_packsmanager.py. Maybe github is getting overworked?

edit: it was just lagging, commits are coming through 👍

cadenmyers13 · 2025-10-09T15:55:23Z

@sbillinge Also, I wrote the function for this tests as well so that is ready for review under packsmanager.py

tests/conftest.py

cadenmyers13 · 2025-10-09T16:31:50Z

tests/test_cli.py

+        # 6a) user wants to copy all examples from all packs to cwd
+        # 6b) user wants to copy all examples from all packs to a target dir
+        (),
+    ],


@sbillinge ready for review here^^^^. Some use cases I jotted down. Let me know what you think.

cadenmyers13 · 2025-10-09T16:34:44Z

@sbillinge jotted down some UCs for the copy_examples on this PR. on second thought however, maybe we want to merge the other test first and do the copy_exmaples test on a separate PR (if so, I will clean up this branch a bit).

sbillinge

next round

sbillinge · 2025-10-09T16:08:14Z

src/diffpy/cmi/packsmanager.py

+        # root_path option provided for testing
+        else:
+            self.packs_dir = Path(root_path).resolve()
+            self.examples_dir = Path(root_path).resolve()


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.

sbillinge · 2025-10-09T16:11:11Z

tests/conftest.py

+    """
+    examples_dir = tmp_path_factory.mktemp("examples")
+
+    # case 1: pack with no examples


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

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

sbillinge · 2025-10-09T16:12:05Z

tests/test_cli.py

+from diffpy.cmi.packsmanager import PacksManager
+
+
+def test_print_info(temp_path, capsys):


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

sbillinge · 2025-10-09T16:12:54Z

tests/test_packsmanager.py

+    # 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 {}


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

sbillinge · 2025-10-09T16:14:30Z

tests/test_packsmanager.py

+        "case3",
+        {
+            "packA": [
+                ("my_ex1", "case3/packA/my_ex1"),


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.

sbillinge · 2025-10-09T16:23:48Z

tests/test_packsmanager.py

+    # Ensure the example directory is correctly set
+    assert pkmg.examples_dir == tmp_ex_dir
+    actual = pkmg.available_examples()
+    # Verify that the keys (pack names) are correct


please delete gratuitous comments

sbillinge · 2025-10-09T16:25:29Z

tests/test_packsmanager.py

+    actual = pkmg.available_examples()
+    # Verify that the keys (pack names) are correct
+    assert set(actual.keys()) == set(expected.keys())
+    # Verify that each expected example exists in actual


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

sbillinge · 2025-10-09T16:34:19Z

tests/test_packsmanager.py

+    # Verify that the keys (pack names) are correct
+    assert set(actual.keys()) == set(expected.keys())
+    # Verify that each expected example exists in actual
+    for expected_pack, expected_list in expected.items():


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.

implemented something similar

sbillinge · 2025-10-09T16:35:20Z

tests/test_packsmanager.py

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


this is good

sbillinge · 2025-10-09T16:35:29Z

tests/test_packsmanager.py

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


cadenmyers13 · 2025-10-09T21:54:22Z

@sbillinge ready for review. Heres the temp tree structure. temp0 is somewhat of a "hidden" dir and each caseX dir gets treated as the top level during each test.

temp0/
├── case1/
│   ├── docs/
│   │   └── examples/
│   │       └── empty_pack/
│   └── requirements/
│       └── packs/
│
├── case2/
│   ├── docs/
│   │   └── examples/
│   │       └── full_pack/
│   │           ├── ex1/
│   │           │   └── solution/
│   │           │       └── diffpy-cmi/
│   │           │           └── script1.py
│   │           └── ex2/
│   │               └── random/
│   │                   └── path/
│   │                       ├── script1.py
│   │                       └── script2.py
│   └── requirements/
│       └── packs/
│
├── case3/
│   ├── docs/
│   │   └── examples/
│   │       ├── packA/
│   │       │   ├── ex1/
│   │       │   │   └── script1.py
│   │       │   └── ex2/
│   │       │       └── solutions/
│   │       │           └── script2.py
│   │       └── packB/
│   │           └── ex3/
│   │               └── more/
│   │                   └── random/
│   │                       └── path/
│   │                           ├── script3.py
│   │                           └── script4.py
│   └── requirements/
│       └── packs/
│
├── case4/
│   ├── docs/
│   │   └── examples/        
│   └── requirements/
│       └── packs/
│
└── case5/
    ├── docs/
    │   └── examples/
    │       ├── packA/
    │       │   └── ex1/
    │       │       └── path1/
    │       │           └── script1.py
    │       └── packB/
    │           └── ex1/
    │               └── path2/
    │                   └── script2.py
    └── requirements/
        └── packs/

… this PR

sbillinge · 2025-10-10T13:39:05Z

This directory structure looks great, thanks.

This is getting a bit hard to review. Please could we make a few separate PRs? Let's make one PR that creates the directory structure fixture. It will need a news and should pass tests (it may fail codecov, that is ok).

THen make one PR per test and we work to get everything green.

Makes these PRs from a clean PR, though where necessary you can copy over code from here.

This needs a news.

cadenmyers13 · 2025-10-10T13:48:10Z

@sbillinge good idea yeah i can do that right now. Do you want to keep this PR or should we just close it so the commit history is clean?

sbillinge · 2025-10-10T13:55:40Z

This can be closed. Let's only reuse code judiciously from it but treat it more as a learning/design exercise. I think we came up with a good design now, though the code and the commit history itself could be cleaner. Working of off clean PRs from main can fix that.

Please make sure PRs are passing everything before reesting a review, but have one function tested per PR, sthg like that.

cadenmyers13 · 2025-10-10T14:24:36Z

@sbillinge okay I've made two PRs, one with the temp dir fixture and the other with the function and test

temp dir fixture: #54 (ready for review)

function and test: #55

cadenmyers13 · 2025-10-10T14:29:59Z

closing to keep PR list clean

initial listing functions for cli

54974ba

cadenmyers13 added 2 commits October 6, 2025 15:31

initial tests

c9cf238

test for listing available examples

f9a14c3

test pretty print

71423d5

api for copying packs and examples

a1ee907

cadenmyers13 commented Oct 6, 2025

View reviewed changes

src/diffpy/cmi/cli.py Outdated Show resolved Hide resolved

cadenmyers13 commented Oct 6, 2025

View reviewed changes

sbillinge reviewed Oct 7, 2025

View reviewed changes

cadenmyers13 added 4 commits October 6, 2025 22:08

have one function for copying examples

b16c90b

move print_info to cli, add params to docstring in available_examples

15c8111

add fnferror to api

db2ac9a

update available_examples test

9fcef71

cadenmyers13 commented Oct 7, 2025

View reviewed changes

tests/test_packsmanager.py Outdated Show resolved Hide resolved

sbillinge reviewed Oct 7, 2025

View reviewed changes

cadenmyers13 added 9 commits October 7, 2025 13:34

have pkmg methods take temp_path instead of pkmg

c70ca27

test print info and test copy examples

d32201a

checkpoint1 commit of working available examples tests

fbbf88c

checkpoint conftest, build example_cases fixture

a458c5e

checkpoint packsmanager.py file

736b519

checkpoint: conftest is building correctly

0bd26fc

checkpoint, working version that gets examples dir outside of PacksMa…

4be38f6

…nager

checkpoint: _get_examples_dir moved back in PacksManager and paths ar…

280390c

…e being found. the keys of actual and expected are asserting True

forgot to add conftest to previous checkpoint commit, so its added in…

99c4951

… this commit

checkpoint: test that the path and examples match

aa65d02

checkpoint: add all 4 cases to test_packsmanager

4aa15b0

sbillinge reviewed Oct 8, 2025

View reviewed changes

cadenmyers13 added 3 commits October 9, 2025 11:11

Add case 5, split into separate tests, and refactor code for style/re…

cfb2273

…adability

sort packs and examples when building dict with available_examples()

fdf09b2

add case 5 to conftest

53ac9a2

cadenmyers13 added 2 commits October 9, 2025 11:23

remove printed fail statement

ce1efa7

rm unused helper function I made in conftest

c39dfae

cadenmyers13 added 2 commits October 9, 2025 12:04

add comment for case5 ex

c2e8076

UCs for copy_examples tests

d1038b7

cadenmyers13 commented Oct 9, 2025

View reviewed changes

tests/conftest.py Show resolved Hide resolved

cadenmyers13 commented Oct 9, 2025

View reviewed changes

sbillinge reviewed Oct 9, 2025

View reviewed changes

cadenmyers13 added 2 commits October 9, 2025 17:47

treat each 'caseX' as the root dir, build out from there

85bb747

change wording for comment

0d08dbe

cadenmyers13 added 3 commits October 9, 2025 17:55

update innit to accept root_path

fcbb1a2

better function name

52aa4e7

revert some files to upstream/main, doing only dict building tests on…

e8c8ee5

… this PR

cadenmyers13 changed the title ~~feat: update cli~~ feat: build examples dict for cli commands Oct 10, 2025

cadenmyers13 closed this Oct 10, 2025



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

		from diffpy.cmi.packsmanager import PacksManager


		def test_print_info(temp_path, capsys):



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

feat: build examples dict for cli commands #53

feat: build examples dict for cli commands #53

Uh oh!

Conversation

cadenmyers13 commented Oct 6, 2025

Uh oh!

github-actions bot commented Oct 6, 2025

Uh oh!

cadenmyers13 commented Oct 6, 2025

Uh oh!

cadenmyers13 commented Oct 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Oct 7, 2025

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge commented Oct 7, 2025

Uh oh!

cadenmyers13 commented Oct 8, 2025

Uh oh!

cadenmyers13 commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 Oct 7, 2025 •

edited

Loading

cadenmyers13 commented Oct 8, 2025 •

edited

Loading

cadenmyers13 Oct 9, 2025 •

edited

Loading

cadenmyers13 commented Oct 9, 2025 •

edited

Loading