-
Notifications
You must be signed in to change notification settings - Fork 1
feat: create app for diffpy.cmi CLI #20
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
=======================================
Coverage 75.67% 75.67%
=======================================
Files 3 3
Lines 37 37
=======================================
Hits 28 28
Misses 9 9 🚀 New features to boost your workflow:
|
@sbillinge ready for review with no force pushes. @bobleesj What do we think about the |
@cadenmyers13 i think
|
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.
please see comments. I think we want to use argparse (we may need it later anyway) and reuse our existing version.py, so most of this can be done without in that way.
@@ -48,6 +48,9 @@ include = ["*"] # package names should match these glob patterns (["*"] by defa | |||
exclude = [] # exclude packages matching these glob patterns (empty by default) | |||
namespaces = false # to disable scanning PEP 420 namespaces (true by default) | |||
|
|||
[project.scripts] | |||
diffpy-cmi = "diffpy.cmi.app:main" |
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.
shall we also have one that is just cmi
(for lazy people). It can run the same function.
Also, what happens if you just type diffpy-cmi
or cmi
without -v
or -h
? Let's test that and make sure it behaves in a nice friendly way.
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.
@sbillinge just typing diffpy-cmi
also returns the same thing as the -h
command.
shall we also have one that is just cmi (for lazy people). It can run the same function.
Yeah we can do that!
src/diffpy/cmi/app.py
Outdated
print( | ||
"""\ | ||
|
||
DiffPy-CMI is our complex modeling framework. It is a highly flexible library |
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.
how about "is a complex modelling...."
Sadly, this terminology never got widely adopted. Many people call it "multi-modal analysis" so we could say something like: "Welcome to diffpy.cmi, a complex modeling infrastructurel for multi-modal analysis of scientific data form different sources.
It is a highly flexible....."
It is not for nanostructure analysis, it can be used for anything....even predicting stock prices. We can say something like that, but mention that modules currently exist for studies of structure from PDF and SAS data, with mroe to be added later. sthg like that?
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.
@sbillinge Okay got it. I copy/pasted this from the current diffpy-cmi documentation fyi. I'll add these changes
src/diffpy/cmi/app.py
Outdated
diffpy-cmi [--version] [--help] | ||
|
||
Options: | ||
-V, --version Show version and exit |
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.
why is the V capitalized? Is that some standard coming from Python? If not, let's lower-case it.
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.
@sbillinge I capitalized because -v
typically stands for verbose. I don't think it matters too much so we could change it to -v
or simply remove the short-hand and require users to do --version
. I feel like when I'm looking for a package version i always type --version
anyways.
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.
yes, let's remove the shortcut to remove confusion
src/diffpy/cmi/app.py
Outdated
|
||
|
||
def version(): | ||
from diffpy.cmi.version import __version__ |
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.
why is this import not at the top of the file? Also, don't we have a version.py
? Is there a reason we are defining a new one?
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.
Yeah this should be fine at the top of the file as well. I'll move it.
Also, don't we have a
version.py
?
yeah in version.py
, __version__
is defined and returns the version number. This just imports that version from the file so we aren't defining a new one.
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.
so we aren't defining a new one.
but I see def version():
which seems to contradict your 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.
Oh I see what youre saying, I'll change the function name
@bobleesj What I mean when a package is created (ie |
@cadenmyers13 yes, I understand. Feel free to make an issue an PR? |
@bobleesj will do, once this is merged I'll do that so we have a good idea of what we want. |
@sbillinge ready for review |
I had to change the requirements file name because of the change made to tests-on-pr here: https://github.com/scikit-package/release-scripts/blob/v0/.github/workflows/_tests-on-pr.yml |
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.
please see comments
src/diffpy/cmi/__init__.py
Outdated
@@ -12,7 +12,8 @@ | |||
# See LICENSE.rst for license information. | |||
# | |||
############################################################################## | |||
"""Complex modeling infrastructure: a modulare framework for multi-modal modeling of scientific data.""" | |||
"""Complex modeling infrastructure: | |||
a modulare framework for multi-modal modeling of scientific data.""" |
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.
modulare?
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.
Typo must've been made when package was created, Ive fixed that everywhere its found
src/diffpy/cmi/app.py
Outdated
|
||
|
||
def main(): | ||
try: |
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.
please do this work using argparse
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.
done
@sbillinge ready for review. I added a function that returns a shorter message if an unknown flag is given:
I've tested this with |
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.
please see my comment
|
||
from diffpy.cmi.version import __version__ | ||
|
||
|
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.
the reason to use argparse is that it does most of this work for you. It will print usage and help so you don't need those functions. Something like
parser = argparse.ArgumentParser(
description="help/usage message"
)
parser.add_argument(
"--version",
action="store_true"
help="Show the program's version number and exit",
)
args = parser.parse_args()
if args.version:
print("version message")
else:
print("help/intro message")
should do everything you want
@sbillinge Ah i see, with some help from chatgpt, this is ready for review. After testing with |
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.
please see my questions about code style, but also, please could you paste what we wee when you type diffpy-cmi
and also cmi
. For one of them do all the options in the matrix:
cmi
cmi --version
cmi -h
for the other we are just testing that it works. I see it as an endpoint in the toml but does it have to be explicitly listed in the arg parser too? not sure. You may have to reinstall the package to get it to work. I expect it is installed as -e
but if you change the package metadata I think all bets are off.....
src/diffpy/cmi/app.py
Outdated
), | ||
formatter_class=argparse.RawDescriptionHelpFormatter, | ||
) | ||
|
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.
is pre-commit doing this or can we close up these blank lines? Currently,
this
is a
bit too
spread out for my liking....
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.
precommit does a cr for the parenths but i removed the lines
@sbillinge no you only need to mention it in the toml
test case for unexpected arg
similar commands work the same for Ready for review |
closes #7
New CLI commands
-V
and-h
. Also link to docs is presented.