-
Notifications
You must be signed in to change notification settings - Fork 5
refactor: Cli commands #76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…lly to make sure temp envs are removed locally
| copytree(src, dest) | ||
| return dest | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced all above copying code with code from PacksManager. Bascially all the work over the last few weeks replaces the above.
| ) | ||
| if not localpath.is_file(): | ||
| plog.info("Manual files not found, falling back to online version.") | ||
| url = "https://www.diffpy.org/products/diffpycmi" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified so it just takes you to diffpy.org page
|
@sbillinge ready for review. Left some inline comments to hopefully make reviewing easier |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
+ Coverage 92.47% 92.70% +0.23%
==========================================
Files 3 3
Lines 186 192 +6
==========================================
+ Hits 172 178 +6
Misses 14 14
🚀 New features to boost your workflow:
|
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good. I think the help statements need a few more words describing what packs and profiles are. As it stands the help is helpful if you know this, but not so helpful if you don't. Giving users hints about how to proceed is also helpful in a help. We can do this by using a similar structure to our warnings "what went wrong. What to do next to try and fix it" . Help is like that but for usage.
Please could you go through all the info -h's that you copy-pasted in the convo (thanks for that....super helpful....also thanks for the inline comments explaining things....also super helpful....more like this!) and try and look at them from the point of view of someone who doesn't actually know what the words mean?
But the structure is now super good. Ready to merge when we get these cosmetics right (and also figure out what is wrong in the test tmdir)
|
@sbillinge ready for review. Here's what the two new help messages look like |
|
Those help messages were great. On a future PR, please just make sure that punctuation and capitalization is consistent across them. I only saw one issue (missing period if our style is to place periods) but I didn't systematically scan for that. Please ask chatgpt is the open source standard is to punctuate/capitalize these or not and let's follow the standard (and standardize across the group software moving forward) |
Updated cli commands from conversation recorded here: https://github.com/bg-mission-control/mission-control/issues/5
I've listed the outputs of commands below. I also tested the copy feature with different target dirs and force permutations.
All
-hcommandsnon
-hcommands