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

Implement the CLI with Click instead of argparse #297

Closed
2 of 3 tasks
csadorf opened this issue Feb 20, 2020 · 9 comments
Closed
2 of 3 tasks

Implement the CLI with Click instead of argparse #297

csadorf opened this issue Feb 20, 2020 · 9 comments
Assignees
Labels
Click Related to refactoring CLI with Click. expertise-needed Extra attention is needed refactor Code refactoring stale
Milestone

Comments

@csadorf
Copy link
Contributor

csadorf commented Feb 20, 2020

Summary

Implement the command line interface (CLI) with the Click library instead of argparse.

Motivation

Using Click for the implementation of a CLI has numerous advantages over using argparse, including a more concise API, the enforcement of a more consistent interface, and built-in support for the implementation of test runners. In addition, the library comes with a collection of helper functions and tools, that are relevant for CLI development. Please see this page for a more comprehensive overview of why one would want to implement a CLI with Click.

Proposed plan for implementation

Since this is a larger refactoring project, it should be executed in stages. Each stage should be introduced as a separate pull request:

  • Ensure that the coverage of signac/__main__.py is measured, see instructions here.
  • Review existing unit tests for signac/__main_.py and implement missing tests to reach a test coverage of at least 80%.
  • Refactor the CLI implementation, replacing argparse with Click exclusively for the 'find' sub command; mark all unit tests for other sub-commands as expected failures. Once the find sub-command was successfully refactored, refactor all other sub-commands. Replicate the unit tests for each command found in tests/test_shell.py in a seperate test module called tests/test_main.py with Click's built-in test runner. The original tests may not fail!

Risks

It is very likely that not all sub-commands can be implemented in the exact same way as before, because Click imposes some restrictions that argparse does not. These restrictions will likely improve the consistency and quality of the CLI, but make it a bit more difficult to create an exact replacement and may require the introduction of some minor backwards incompatibilities.

@csadorf csadorf added enhancement New feature or request expertise-needed Extra attention is needed labels Feb 20, 2020
@csadorf
Copy link
Contributor Author

csadorf commented Feb 20, 2020

@vishav1771 Would you be interested in giving this a shot?

@vishav1771
Copy link
Contributor

Sure 👍

@csadorf csadorf added the Click Related to refactoring CLI with Click. label Feb 20, 2020
@csadorf
Copy link
Contributor Author

csadorf commented Feb 20, 2020

Sure

Awesome, thx! Please feel free to discuss questions about this project directly on this issue. Please apply the "Click" label to any PRs related to this issue so that it is easier for us to keep track of things.

@bdice
Copy link
Member

bdice commented Feb 20, 2020

@vishav1771 Thanks for taking this on! Eventually we will want to do the same for signac-flow, so it may be helpful to document the steps you take along the way for a contributor to do this again for that package.

@vishav1771
Copy link
Contributor

vishav1771 commented Feb 22, 2020

To measure coverage of signac/__main__.py: pytest provides pytest-cov extension which can be used. Should I go with this?

@vyasr
Copy link
Contributor

vyasr commented Feb 25, 2020

I think using pytest-cov is a good idea. We will probably want to use click.testing for actually writing future tests.

@bdice bdice added refactor Code refactoring and removed enhancement New feature or request labels Feb 27, 2020
@csadorf csadorf linked a pull request Mar 2, 2020 that will close this issue
12 tasks
@csadorf csadorf removed a link to a pull request Mar 2, 2020
12 tasks
@csadorf
Copy link
Contributor Author

csadorf commented Mar 2, 2020

I unlinked #300 again, because merging that PR will not fully close this issue.

@bdice bdice added this to the v1.5.0 milestone Mar 16, 2020
@csadorf
Copy link
Contributor Author

csadorf commented Mar 19, 2020

I've marked the first two tasks as completed with the merge of #300 . Nice job @vishav1771 !

@stale
Copy link

stale bot commented Mar 31, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 31, 2021
@stale stale bot closed this as completed Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Click Related to refactoring CLI with Click. expertise-needed Extra attention is needed refactor Code refactoring stale
Projects
None yet
Development

No branches or pull requests

4 participants