feat: place examples in pack directories for cli #49

cadenmyers13 · 2025-09-23T18:16:11Z

This makes the cli versatile for new pack additions. See screenshots for functionality and failure.

cadenmyers13 · 2025-09-23T18:18:14Z

@sbillinge ready for review

codecov · 2025-09-23T18:39:18Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.16%. Comparing base (61f353e) to head (d53fab8).
⚠️ Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   81.73%   82.16%   +0.42%     
==========================================
  Files          11       12       +1     
  Lines        1210     1239      +29     
==========================================
+ Hits          989     1018      +29     
  Misses        221      221

Files with missing lines	Coverage Δ
.../ch03NiModelling/solutions/diffpy-cmi/fitBulkNi.py	`97.54% <ø> (ø)`
...df/ch03NiModelling/solutions/diffpy-cmi/fitNPPt.py	`86.86% <ø> (ø)`
...Modelling/solutions/diffpy-cmi/helper_functions.py	`16.03% <ø> (ø)`
...es/pdf/ch05Fit2Phase/solutions/diffpy-cmi/fit2P.py	`90.14% <ø> (ø)`
...StructureGen/solutions/diffpy-cmi/fitCrystalGen.py	`89.28% <ø> (ø)`
...haseTransitions/solutions/diffpy-cmi/fitTSeries.py	`89.85% <ø> (ø)`
...ch08NPRefinement/solutions/diffpy-cmi/fitCdSeNP.py	`89.07% <ø> (ø)`
...f/ch11ClusterXYZ/solutions/diffpy-cmi/fitCdSeNP.py	`90.24% <ø> (ø)`
tests/test_cli.py	`100.00% <100.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Updated the examples section to include specific pack names and modified the directory structure for clarity.

sbillinge

please see comments

sbillinge · 2025-09-24T01:01:49Z

src/diffpy/cmi/cli.py

-    return sorted([p.name for p in root.iterdir() if p.is_dir()])
+        return {}
+
+    packs: dict[str, list[str]] = {}


if you are using chatgpt pleaes review the code very carefully. We are not using type-hints atm so remove all type hints. Also, this is a lot of code now. Is it all necessary or there is a more elegant way of accomplishing the same thing?

My life gets way harder when I have to review crappy code written by chatgpt. At least when I spend time reviewing crappy code written by students I feel that the student is learning and in the future, as a result, I will have to spend less time reviewing.....

Are there tests for this code?

@sbillinge sorry about that... I do review any gpt generated code and try my best to use it as a tool and not to be lazy. I'll scrutinize it more in the future.

Here's my idea for how users can see and copy the examples. I want the examples to be listed under each pack like you see in the screenshot above where its something like this:

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

This code accomplishes this by creating a dict and storing the pack dir names as keys and a list of the examples as the value. Then, I want users to be able to copy the example to there cwd via cmi copy example pack1/exampleA. The upstream configuration copies examples by running cmi example copy exampleA, as more packs get added to cmi you could imagine how this would become a mess. You may have interpreted this from this PR already but I just want to be explicit in my objectives. Can you explain a bit more why you think this code is crappy?

Tieqiong wrote a CI test for this here that runs the commands but i don't see any tests for listing examples (cmi example list) or copying them (cmi example copy <ex name>)

sbillinge · 2025-09-25T18:21:42Z

Let me know when the is ready for review per our discussion @cadenmyers13

cadenmyers13 · 2025-09-25T18:26:22Z

@sbillinge working on it rn. will do 👍

cadenmyers13 · 2025-09-25T19:27:04Z

src/diffpy/cmi/cli.py

    """
-    src = _installed_examples_dir() / example
+    if "/" not in pack_example:
+        raise ValueError("Example must be specified as <pack>/<exdir>")


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

cadenmyers13 · 2025-09-25T19:29:26Z

src/diffpy/cmi/cli.py

+        for pack, examples in get_examples_by_pack().items():
+            print(f"{pack}:")
+            for ex in examples:
+                print(f"  - {ex}")


prints examples in the format of

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

instead of

$ cmi example list exampleA exampleB exampleC exampleD

cadenmyers13 · 2025-09-25T19:30:59Z

src/diffpy/cmi/cli.py

+        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


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.

cadenmyers13 · 2025-09-25T19:32:01Z

@sbillinge okay this is ready for review. i changed some function and variable names to reflect better what is happening. See inline for descriptions

sbillinge

I like to review tests before I review code but i don't see tests for these new functions?

cadenmyers13 · 2025-09-25T20:00:00Z

@sbillinge heard, on it

sbillinge · 2025-09-25T20:08:02Z

super. fyi, this is conflicted now. i expect that this is where you deleted the junk then couldn't figure out why it reappeared :). You can just merge main into this branch after syncing main.

cadenmyers13 · 2025-09-25T20:22:24Z

@sbillinge Oh yeah, will do. Thanks for the heads up!

cadenmyers13 · 2025-09-25T21:18:55Z

@sbillinge Added tests, I had chatgpt help me with this but the logic makes sense to me. Basically, I'm just checking to see if the copied dir is copied, outputs are printed, etc.. I was able to get the tests to fail by changing the dirs. the "copy" test uses monkeypatch and the "list" test does not. I'm not sure which one would be preferred here. I assume probably not monkeypatch since we might want to test an actual existing directory. I ran into some bugs trying to do that. I have to head out right now, but if you let me know which one you prefer (if either) and i can make the changes tomorrow

sbillinge

Couple of comments. The tests seem to be all testing main. Can you break out functionality into functions and test them separately?

Also, I want to standardize on using pytest.mock rather than monkypatch

sbillinge · 2025-09-25T23:42:11Z

here are kind of the steps:

decide what behavior you want the function to encode
give the function a reasonable name that captures the behavior (things like get_something etc. It will be some kind of action
take a guess at the signature (inputs and outputs) and type them into the function
write the docstring, capturing the behavior in words.

I usually find that steps 1-4, my thinking evolves and changes so it can be a slightly iterative process

write the test function but make it empty to begin with (the function is so far empty too)
write a comment for all the different behaviors. We use a style # example input. Expected behavior.
write tests. make use of pytest.parametrize to test all the successful test cases (if there are more than one). We generally put tests that test exception handling in another separate test function
They will fail because you didn't write hte function yet.
Get the tests reviewed and finalized
Write the function till the tests pass

because you wrote the functions before the tests this will go a bit differently here, but don't write the tests so functions you worte pass, but write tests that give you the behavior you want, then write the function so the tests pass. Then you are sure you have the right behavior

cadenmyers13 · 2025-09-26T14:33:42Z

@sbillinge Thanks for the detailed instructions. Very helpful! I think I'm starting to get the hang of it a bit. I made a push testing map_pack_to_examples. I'll continue writing tests, but let me know if this looks okay to you in the meantime.

sbillinge

pls see comments. Just working on tests atm

sbillinge · 2025-09-26T15:27:31Z

tests/test_cli.py

+@pytest.mark.parametrize(
+    "structure, expected",
+    [
+        # case: one pack with one example


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.

sbillinge · 2025-09-26T15:29:44Z

tests/test_cli.py

+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


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.

sbillinge · 2025-09-26T15:30:37Z

tests/test_cli.py

-    assert rc == 0
-    assert (cwd / "ex1").exists()
+@pytest.mark.parametrize(
+    "structure, expected",


can we change structure to inputs or sthg like that?

sbillinge · 2025-09-26T15:40:59Z

tests/test_cli.py

+        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)


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

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

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." ) ```

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

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.

cadenmyers13 · 2025-09-26T18:38:22Z

@sbillinge Ready for review. I've added tests for map_pack_to_example and copy_example. I've left the mocker in there. please see response to your comment on this, and we can figure out how it should be handled.

cadenmyers13 · 2025-09-29T14:10:58Z

@sbillinge ready for review. This is just one test function (working on the rest rn), but I went with a different approach that just checks the output data type

cadenmyers13 · 2025-09-29T14:35:56Z

@sbillinge Okay all tests are added. I think this looks much less hacky

sbillinge

this is pretty far from where it is supposed to be. We may want to spend an hour or two together working on this, so you can get the idea a bit better?

sbillinge · 2025-09-29T18:02:37Z

tests/test_cli.py

+        "pdf/ch03NiModelling",
+    ],
+)
+def test_copy_example_success(tmp_path, input_valid_str):


is this test testing the test? We don't normally do that.

This test is testing the cmi example copy cli functionality, not the "run all scripts" test. Although they both use a temp directory.

sbillinge · 2025-09-29T18:03:12Z

tests/test_cli.py

+    assert actual == expected
+
+
+def test_copy_example_fnferror():


for bad tests we normally call them test_<function_name>_bad

sbillinge · 2025-09-29T18:04:34Z

tests/test_cli.py

+@pytest.mark.parametrize(
+    "input_bad_str",
+    [
+        "",  # empty string


here I see an empty string and then a comment saying # empty string which of course, is not needed. The comments need to be capturing behavior.

sbillinge · 2025-09-29T18:04:38Z

tests/test_cli.py

+    """Test that map_pack_to_examples returns the right shape of
+    data."""
+    actual = cli.map_pack_to_examples()
+    assert isinstance(actual, dict)


These isinstance tests do rather little. The right test is one that tests that the correct things are loaded in the dict. Let's start by just writing comments.

# single pack, single example. <expected output>

cadenmyers13 · 2025-09-29T18:22:15Z

We may want to spend an hour or two together working on this, so you can get the idea a bit better?

@sbillinge Im down to do that. Do you have time today/this evening? I see your calendar is blocked until 6pm.

sbillinge · 2025-09-29T22:28:34Z

We may want to spend an hour or two together working on this, so you can get the idea a bit better?

@sbillinge Im down to do that. Do you have time today/this evening? I see your calendar is blocked until 6pm.

no, it will have to be Friday I think. Perhaps we can do it asynchronously. I could make a PR and push the steps one by one?

sbillinge · 2025-09-29T22:29:02Z

Maybe I will merge this one and make a PR on to that

cadenmyers13 · 2025-09-30T20:08:35Z

@sbillinge We can start asynchronously, but I think it might be good for me to do this with you in real time.

sbillinge · 2025-09-30T22:19:07Z

@sbillinge We can start asynchronously, but I think it might be good for me to do this with you in real time.

let's do both.

Step 1 is to figure out at a very high level what the task is. this is the UC process. Here are some examples:

UC 1
1.  user wants to know what packs are available
2. user asks cmi
3. cmi lists them

UC 2 
1. user wants to know what examples are available
2. users asks cmi
3. cmi lists them

UC 3 
1. user wants to do one of the examples
2. user tells cmi 
3. cmi copies everything to cwd to do the example

UC 4 
1. as UC 3 but user specifies a different target file-path
2. cmi copies everything to target

UC 5
1. as UC 4 but user specifies a list of examples at the same time

UC 6
1. as UC 4 but user wants all the examples from pack X

IC 7 
1. as UC 6 but user wants all the examples from a list of packs

UC 8 
1. as UC 7 but user wants all the examples from everything

Are there any I am missing?

sbillinge · 2025-09-30T22:23:54Z

Next is a discussion of which UCs we want to implement. Roughly speaking let's assume we want them all.

Next is a first cut at architecture. Good to discuss it a bit, but what structure do we want, data objects, etc. to cover as much of the UCs as possible? What is a list of tasks that the code would have to do to accomplish the tasks? e.g.,

generate a dict of packs and examples dynamically from all the examples it finds.
design CLI for the above tasks
write functions that pull from the examples dict for the various listing and copying tasks.

and so on....

Next is discuss and finalize these things and we can start on the code.....

sbillinge · 2025-09-30T22:26:36Z

@cadenmyers13 please do me a favor and move the last few discussion comments above to a new issue for safe keeping.

I will merge this so that I can work on a clean PR. Please add any UCs you can think of or anything to the code design discussion.

cadenmyers13 added 4 commits September 23, 2025 14:04

move examples under pdf dir

0f3293f

edit cli to reflect new path change

c5c9f84

news

1092dcb

fix typo

14102ad

Revise examples in news/ex-path.rst for clarity

d423377

Updated the examples section to include specific pack names and modified the directory structure for clarity.

sbillinge reviewed Sep 24, 2025

View reviewed changes

cadenmyers13 added 2 commits September 24, 2025 22:00

rm type hint

683b4c3

merge to local: move news from added to changed

71f9de4

cadenmyers13 mentioned this pull request Sep 25, 2025

docs: edit installation instructions to match new example structure #50

Closed

cadenmyers13 added 3 commits September 25, 2025 15:07

change list_examples to map_pack_to_examples which returns a dict

3a1ee8b

change variable name to reflect action properly

d3b531c

change function name

73d083e

cadenmyers13 commented Sep 25, 2025

View reviewed changes

sbillinge reviewed Sep 25, 2025

View reviewed changes

update docstring

61a9bd6

Merge branch 'main' of github.com:diffpy/diffpy.cmi into ex-path

012a45e

cadenmyers13 added 2 commits September 25, 2025 17:10

tests for help command, listing examples, and copying examples

d90a271

fix monkeypatch debugging code

c12f6b5

sbillinge reviewed Sep 25, 2025

View reviewed changes

cadenmyers13 added 3 commits September 26, 2025 10:29

update function names to be more readable

c0bead8

test map pack to examples function

63ac630

add no packs, no examples case

e0a2c75

make sure directory was created

81fa5cf

sbillinge reviewed Sep 26, 2025

View reviewed changes

cadenmyers13 added 2 commits September 26, 2025 14:34

add tests for copy_example

10771b1

Add Error if pack or example doesnt exist

4ee36af

cadenmyers13 added 2 commits September 26, 2025 14:48

Merge branch 'main' ex-path to see core example

4019e36

map packs function test, simplified

65fd2c7

cadenmyers13 added 2 commits September 29, 2025 10:34

add condition that catches extra slashes

74e992b

test copy behavior

d53fab8

sbillinge reviewed Sep 29, 2025

View reviewed changes

sbillinge merged commit 53318eb into diffpy:main Sep 30, 2025
5 checks passed

cadenmyers13 deleted the ex-path branch October 1, 2025 13:44

Uh oh!

feat: place examples in pack directories for cli #49

feat: place examples in pack directories for cli #49

Uh oh!

Conversation

cadenmyers13 commented Sep 23, 2025

Uh oh!

cadenmyers13 commented Sep 23, 2025

Uh oh!

codecov bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbillinge left a comment

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!

sbillinge commented Sep 25, 2025

Uh oh!

cadenmyers13 commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 Sep 25, 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!

cadenmyers13 commented Sep 25, 2025

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Sep 25, 2025

Uh oh!

sbillinge commented Sep 25, 2025

Uh oh!

cadenmyers13 commented Sep 25, 2025

Uh oh!

cadenmyers13 commented Sep 25, 2025

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

sbillinge commented Sep 25, 2025

Uh oh!

cadenmyers13 commented Sep 26, 2025

Uh oh!

sbillinge left a comment

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 Sep 26, 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!

cadenmyers13 Sep 26, 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!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Sep 26, 2025

Uh oh!

cadenmyers13 commented Sep 29, 2025

codecov bot commented Sep 23, 2025 •

edited

Loading

cadenmyers13 Sep 25, 2025 •

edited

Loading

cadenmyers13 Sep 26, 2025 •

edited

Loading

cadenmyers13 Sep 26, 2025 •

edited

Loading

cadenmyers13 commented Sep 29, 2025 •

edited

Loading