Skip to content

Conversation

@cadenmyers13
Copy link
Contributor

@cadenmyers13 cadenmyers13 commented Oct 20, 2025

cleaned up version of this PR: #60

closes: #56

@cadenmyers13
Copy link
Contributor Author

@sbillinge ready for review

@sbillinge
Copy link
Contributor

Looks good as long as all the edits were brought over correctly!

@cadenmyers13
Copy link
Contributor Author

@sbillinge compared them side-by-side and the two PRs are the same

@sbillinge
Copy link
Contributor

Let's then implement the method

@cadenmyers13
Copy link
Contributor Author

@sbillinge ready for review. All tests pass except one that captures the bad output. Im still figuring out how to capture the statement, but wanted to make a push so you can leave comments on function styling and such

@cadenmyers13
Copy link
Contributor Author

cadenmyers13 commented Oct 23, 2025

I've included a test for the forced overwrite copy of examples.

@sbillinge
Copy link
Contributor

this still seems to be failing tests

@cadenmyers13
Copy link
Contributor Author

@sbillinge Pytest is passing locally now but this is failing CI for some reason. I'm trying to debug on my fork cadenmyers13#8 and I can't quite figure out whats going on. There is a shell script that runs (requirements/packs/scripts/_pytest.sh) and it calls tar_url.txt which contains this

https://github.com/diffpy/diffpy.srreal/archive/refs/tags/1.4.0.tar.gz
https://github.com/diffpy/diffpy.srfit/archive/refs/tags/3.2.0.tar.gz
https://github.com/diffpy/pyobjcryst/archive/refs/tags/2025.1.0.tar.gz
https://github.com/diffpy/diffpy.structure/archive/refs/tags/3.3.1.tar.gz
https://github.com/Tieqiong/diffpy.utils/archive/refs/tags/3.6.2-rc.0.tar.gz

@Tieqiong Do you know what might be causing this?

FYI, I forgot to pull from upstream before creating this branch so i had to merge the python 3.14 support 🤦‍♂️

@Tieqiong
Copy link
Contributor

Tieqiong commented Oct 29, 2025

@cadenmyers13 seems like tests for srreal and pyobjcryst didn't run, probably not compatible with python 3.14 resulting in installation issue. You can see the test env runs on 3.14, which roots from recent 3.14 update on release-scripts/.github/workflows/_tests-on-pr.yml.
Not quite sure about the ones that got successfully tested though, they are supposed to support only up to 3.13 ... Let's try to add python_version: 3.13 in the test-on-pr workflow for now and see if it passes for now!

@sbillinge
Copy link
Contributor

We also are out of free credits. Even though CMI is open software, it turns out Mac and Windows builds are not free even then, and CMI running all it's examples burned through our allocation. This is also causing tests to fail, at least the merge to main tests

@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.24%. Comparing base (9f47832) to head (f4fe605).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_packsmanager.py 96.15% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
+ Coverage   82.93%   83.24%   +0.30%     
==========================================
  Files          13       12       -1     
  Lines        1313     1349      +36     
==========================================
+ Hits         1089     1123      +34     
- Misses        224      226       +2     
Files with missing lines Coverage Δ
tests/conftest.py 89.53% <100.00%> (+1.86%) ⬆️
tests/test_packsmanager.py 93.75% <96.15%> (+4.46%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cadenmyers13
Copy link
Contributor Author

@Tieqiong Okay, i added python_version: 3.13 and it looks like its working. Ive created an issue to remove this pin when we get the necessary packages up to python 3.14. As @sbillinge mentioned we are out of credits, but the tests pass:

Screenshot 2025-10-30 at 2 52 56 PM

@sbillinge On another note, is this how the warning message should be handled/captured for the case when the user tries to copy to a pre-existing directory?

@sbillinge
Copy link
Contributor

warnings that are raised correctly should pass green in tests. We would like to write a test that tests that a warning is raised correctly.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

This looks good now except hte warning. We should think a bit about this. Let's say there is one file that is duplicate. Do we want the code to crash and not copy anything? Or do we want it to copy everything it can, but just not copy the duplicate and throw a warning? Which is better behavior?

@cadenmyers13
Copy link
Contributor Author

We should think a bit about this. Let's say there is one file that is duplicate. Do we want the code to crash and not copy anything? Or do we want it to copy everything it can, but just not copy the duplicate and throw a warning? Which is better behavior?

@sbillinge I like copying everything it can and throw the warning. I like this because if the user wants a restart on a specific file, they can remove this file then run cmi copy example_name. All other work on any other file will not be overwritten, but they can have a fresh start on the new file. Then, if they want a complete restart, the can run with --force and everything will be overwritten. Doing this the other way doesn't add this extra layer of flexibility. The user would have to completely start over with --force or copy to a different location then manually move whatever files they want over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants