Skip to content
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

Merged
merged 13 commits into from
Nov 9, 2023

Conversation

naik-aakash
Copy link
Collaborator

@naik-aakash naik-aakash commented Oct 22, 2023

Will close #139 and #162

Major Change

  • Arguments have been regrouped and commented (help understanding and modifications in future better)
  • All the actions now will show help correctly (Will show only the files necessary for running it and what options are available specific to that action)

Todo

  • After merging orbital-wise analysis PR, update the final bit.

@naik-aakash naik-aakash changed the title [WIP] refactor cli.py for cleaner options on cli help Refactor cli.py for cleaner options on cli help Oct 24, 2023
@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo , this PR is ready to be reviewed and merged

@JaGeo
Copy link
Owner

JaGeo commented Oct 24, 2023

Maybe later today. Need time for this.

@JaGeo
Copy link
Owner

JaGeo commented Oct 24, 2023

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?

@JaGeo
Copy link
Owner

JaGeo commented Oct 24, 2023

I am also wondering if our naming schema of the commands is really that good:
In this reference, they provide some suggetions, e.g. starting all commands that deal with files with "file-"
https://medium.com/jit-team/guidelines-for-creating-your-own-cli-tool-c95d4af62919

@JaGeo
Copy link
Owner

JaGeo commented Oct 24, 2023

Maybe the following could work:

  • description
  • description-quality
  • plot-automatic, plot-auto (and former names)
  • create-inputs
  • plot-dos
  • plot-icohp-distance
  • plot

?

We coud sort them in a alphabetical order as well.

@naik-aakash
Copy link
Collaborator Author

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 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.

@JaGeo
Copy link
Owner

JaGeo commented Oct 24, 2023

Just wait a few more minutes with changing anyhting. I am currently refining some of the other texts. I will push the changes soon.

@naik-aakash
Copy link
Collaborator Author

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 😃

@JaGeo
Copy link
Owner

JaGeo commented Oct 24, 2023

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.

@naik-aakash
Copy link
Collaborator Author

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

@ajjackson
Copy link
Collaborator

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!

@JaGeo
Copy link
Owner

JaGeo commented Oct 25, 2023

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!

@naik-aakash
Copy link
Collaborator Author

Maybe the following could work:

  • description
  • description-quality
  • plot-automatic, plot-auto (and former names)
  • create-inputs
  • plot-dos
  • plot-icohp-distance
  • plot

?

We coud sort them in a alphabetical order as well.

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",
Copy link
Owner

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.

Copy link
Collaborator Author

@naik-aakash naik-aakash Oct 27, 2023

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?

Copy link
Owner

Choose a reason for hiding this comment

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

-fcohp sounds good

@naik-aakash
Copy link
Collaborator Author

Done with the changes here 😄

@JaGeo
Copy link
Owner

JaGeo commented Oct 27, 2023

I will try to do it next week

lobsterpy/cli.py Outdated Show resolved Hide resolved
lobsterpy/cli.py Outdated Show resolved Hide resolved
lobsterpy/cli.py Outdated Show resolved Hide resolved
lobsterpy/cli.py Outdated
broadening_group.add_argument(
"--sigma",
plotting_group.add_argument(
"-fs",
Copy link
Owner

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.

Copy link
Collaborator Author

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",
Copy link
Owner

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

Copy link
Collaborator Author

@naik-aakash naik-aakash Oct 28, 2023

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

Copy link
Collaborator Author

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",
Copy link
Owner

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

Copy link
Collaborator Author

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.

@JaGeo
Copy link
Owner

JaGeo commented Oct 28, 2023

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.

@naik-aakash
Copy link
Collaborator Author

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.

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 😃

@JaGeo
Copy link
Owner

JaGeo commented Oct 28, 2023

I don't have much experience with this but I recommend keeping the commands short.

@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo , this could be merged as well now

@JaGeo JaGeo merged commit 22923b3 into JaGeo:main Nov 9, 2023
23 checks passed
@naik-aakash naik-aakash deleted the update_cli branch June 20, 2024 06:54
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.

clean up cli argparse
3 participants