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

migrate cli to extern crate #598

Merged
merged 16 commits into from
Oct 12, 2023
Merged

migrate cli to extern crate #598

merged 16 commits into from
Oct 12, 2023

Conversation

juan518munoz
Copy link
Contributor

@juan518munoz juan518munoz commented Oct 9, 2023

CAIRO PROVER CLI

Description

Migration of cli functionallity to an external crate that uses clap4 for argument parsing and handling.

  • Make the CLI it's own package, as to have the cairo-prover as a pure lib, and the cairo-prover-cli as a pure app.
  • Migrate the CLI to Clap 4 (Include help functionality for commands)
image

Type of change

  • Optimization

Checklist

  • Linked to Github Issue
  • This change requires new documentation.
    • Documentation has been added/updated.

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2023

Codecov Report

Merging #598 (581aa3b) into main (5a938e7) will increase coverage by 0.09%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
+ Coverage   95.61%   95.71%   +0.09%     
==========================================
  Files         111      111              
  Lines       19318    19298      -20     
==========================================
  Hits        18471    18471              
+ Misses        847      827      -20     
Files Coverage Δ
provers/cairo-prover-cli/src/main.rs 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@juan518munoz juan518munoz linked an issue Oct 9, 2023 that may be closed by this pull request
4 tasks
@juan518munoz juan518munoz marked this pull request as ready for review October 11, 2023 13:12
@juan518munoz juan518munoz requested review from schouhy, ajgara and a team as code owners October 11, 2023 13:12
@entropidelic
Copy link
Contributor

The PR looks good! nice work.
I have some questions/suggestions:

  • Now that we use clap for the CLI, should we continue using the Makefile CLI? Maybe it is ok to have both, but the interface using clap directly seems nicer to me. In this case, me should add to the docs how to use it directly, without the need of make.
  • Maybe not too relevant right now, but in the future as the number of arguments for the CLI grows, we may want to specify them with names (for example, for specifying the program --program <compiled_program>, etc). This way the intent is clearer and the order of the arguments doesn't matter, making it a little bit friendlier for the user.

cc @MauroToscano

@entropidelic
Copy link
Contributor

entropidelic commented Oct 12, 2023

Lets merge this now and update the readme with CLI usage instructions in another PR

@entropidelic entropidelic added this pull request to the merge queue Oct 12, 2023
Merged via the queue into main with commit 50b9fbd Oct 12, 2023
6 checks passed
@entropidelic entropidelic deleted the cli-improvements branch October 12, 2023 15:27
@juan518munoz juan518munoz mentioned this pull request Oct 12, 2023
3 tasks
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.

Cairo Prover CLI Improvements
5 participants