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

re-write the cli using click (or maybe typer?) #63

Closed
nidhaloff opened this issue Jun 13, 2021 · 11 comments · Fixed by #87
Closed

re-write the cli using click (or maybe typer?) #63

nidhaloff opened this issue Jun 13, 2021 · 11 comments · Fixed by #87
Labels
contribution issue meant for contributors that wants to help growing the project discussion let's discuss a topic about igel easy easy first issue enhancement New feature or request feature new feature good first issue Good for newcomers help wanted Extra attention is needed

Comments

@nidhaloff
Copy link
Owner

Description

I'm the creator and only maintainer of the project at the moment. I'm working on adding new features and thus I would like to let this issue open for newcomers who want to contribute to the project.

Basically, I wrote the cli using argparse since it is part of the standard language already. However, I'm starting to rethink this choice because it has some issues that the click library already overcome.

With that said, it would be great to re-write the cli in click or even in typer, which also uses click under the hood but adds more features.

If someone wants to work on this, please feel free to start directly, you don't need to ask for permission.

PS: Feel free to suggest other libraries. I just suggested click since I'm familiar with it

I hope not but If this issue stayed open for a long time, then I will start working on it myself

@nidhaloff nidhaloff added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers easy easy first issue contribution issue meant for contributors that wants to help growing the project feature new feature discussion let's discuss a topic about igel labels Jun 13, 2021
@rmallof
Copy link

rmallof commented Jun 13, 2021

Hi. Adding my 2 cents: maybe https://github.com/google/python-fire can help speed up the process :)

@nidhaloff nidhaloff pinned this issue Jun 13, 2021
@RackReaver
Copy link

I use docopt for most of my projects now as click is usually overkill for what I need.

Based on what I see I think click is probably going to be your best option for your project as it has great modularity.

@nidhaloff
Copy link
Owner Author

@rmallof @RackReaver I'm not familiar with the two packages but thanks for the suggestion.

Yes I think click is a good choice for igel. Maybe typer would be even better but I'm not sure. I must check it out but it is also based on click and has similar syntax so that's the advantage

@RackReaver
Copy link

I'm not super familiar with ML but I'd be willing to give converting this a whirl. Do you have existing tests for the cli to ensure nothing is missed?

@nidhaloff
Copy link
Owner Author

nidhaloff commented Jun 21, 2021

@RackReaver At the moment there are no tests for the cli, but even then, it must be updated/adapted to the click syntax. I implemented the first simple version using argparse, which had limitations and this is exactly one of the reasons why I want to switch to click.

If you want to give it a try, I would happily review and eventually update your PR. You don't have to be familiar with ML, the cli code has nothing to do with ML, it's just python.

@shubham3174
Copy link

Hi @nidhaloff, I am new to open source and would like to start working on it. I would try to replace argparse with click in cli.py

@akshaynarisetti
Copy link
Contributor

@nidhaloff Could you please assign the issue under me, I have begun working on rewriting the code.

@kuspia
Copy link
Contributor

kuspia commented Aug 20, 2021

@nidhaloff can u please share what were the issues you faced with argparse and why we need to rewrite it in click maybe I can fix that in argparse itself.

@nidhaloff
Copy link
Owner Author

@akshaynarisetti I don't have to assign you the issue. Just work on it and make a PR. Many people told me in the past that they started working on it, but I still did not receive any PR from them, so I will not assign the issue to anyone. Just make a PR when you are done.

@kuspia there are no issues with argparse. Click is just convenient to use, the code would be cleaner. I also like the click group feature, where you can group many sub commands under one parent command https://click.palletsprojects.com/en/1.x-maintenance/api.html#click.option
If you have something in mind that you can improve using argparse, then create an issue for it and make a PR ;)

@GouravWadhwa
Copy link

Hi @nidhaloff, I have wrote something for command line arguments using click. I am still not sure how to integrate it and would really appreciate some suggestions. You can access the code from here https://github.com/GouravWadhwa/igel/blob/master/igel/click_cli.py.

@nidhaloff nidhaloff linked a pull request Aug 26, 2021 that will close this issue
@nidhaloff nidhaloff unpinned this issue Aug 26, 2021
@kuspia
Copy link
Contributor

kuspia commented Aug 26, 2021

Hi @nidhaloff kindly check #89 it really enhances the cli.py using click version, I was working on it, if you will consider it, it will be my pleasure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution issue meant for contributors that wants to help growing the project discussion let's discuss a topic about igel easy easy first issue enhancement New feature or request feature new feature good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants