-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refactor cli.py for cleaner options on cli help #172
Conversation
Hi @JaGeo , this PR is ready to be reviewed and merged |
Maybe later today. Need time for this. |
Hi @naik-aakash , I think this already looks great. I was wondering if one should introduce very short options in the cli as well (see here https://nullprogram.com/blog/2020/08/01/) I am not an expert with cli design. @ajjackson, do you maybe have time to your give opinion on this? |
I am also wondering if our naming schema of the commands is really that good: |
Maybe the following could work:
? We coud sort them in a alphabetical order as well. |
I think introducing very short options are not a good idea, could lead to more confusion. But naming conventions , I can try to find more besides the resources you shared. |
Just wait a few more minutes with changing anyhting. I am currently refining some of the other texts. I will push the changes soon. |
Sure, am not actively on the code now , so no rush 😃 |
I have updated and simplified the language in the description a bit. I don't think "will ...." is very helpful here. Maybe, check yourself. And, my recommendations for new argument names are above. I think this would simplify the interface a lot. |
Thanks! Your changes reads much better |
I am on parental leave at the moment. I can try to give this a look over at some point but can't promise precisely when! |
Don't worry! We will probably continue as described above. If you have time or are back from parental leave, we are happy to include further advice, recommendations! |
Done, I sorted them and tried to do this for sub-args where it was possible as well |
default="lobsterout", | ||
coxxcar_file = argparse.ArgumentParser(add_help=False) | ||
coxxcar_file.add_argument( | ||
"--file-cohpcar", |
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.
While this is cleaner, we should leave cohpcar as an alias in any case.
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.
Should we have such an option for all others as well?
I was thinking maybe I add a shorthand alias like -fcohp
or so for all the file-related args. Could be much more intuitive?
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.
-fcohp sounds good
Done with the changes here 😄 |
I will try to do it next week |
lobsterpy/cli.py
Outdated
broadening_group.add_argument( | ||
"--sigma", | ||
plotting_group.add_argument( | ||
"-fs", |
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.
Can you check elsewhere what they use?
Also, the f might be confusing now with the files.
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.
removed this short notation
@@ -224,7 +239,15 @@ def get_parser() -> argparse.ArgumentParser: | |||
"stylesheets when using --style." | |||
), | |||
) | |||
plotting_group.add_argument("--title", type=str, default="", help="Plot title") | |||
plotting_group.add_argument( | |||
"-sty", |
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.
Wondering what other codes use
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.
Thanks! Will check it out
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.
Could not find this in other codes, I think it should be fine
plotting_group.add_argument( | ||
"--width", type=float, default=None, help="Plot width in inches" | ||
"-wd", |
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.
Same comment as before. I guess standards should exist for the short notation
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.
I checked into this, usually -w is used. The short notation for height
arg will be -h
which creates a conflict with default -h
arg for helo. So I feel -wd
and -ht
will keep it bit consistent.
Thanks. Looks much better already. I am wondering if there are standards in the community for short commands for font size, style etc And, if you have a dos plotter, you can orobably just say -addtotal instead of addtotaldos etc. |
Oh yes , Thanks! Will change it 😃 |
I don't have much experience with this but I recommend keeping the commands short. |
Hi @JaGeo , this could be merged as well now |
Will close #139 and #162
Major Change
Todo