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

chore: adding CLI interface #22

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented Apr 8, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 19.10%. Comparing base (00c3775) to head (3e77dc3).

Files Patch % Lines
crates/committer_cli/src/main.rs 0.00% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
- Coverage   22.36%   19.10%   -3.27%     
==========================================
  Files           7        7              
  Lines          76       89      +13     
  Branches       76       89      +13     
==========================================
  Hits           17       17              
- Misses         59       72      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)

a discussion (no related file):
based on this


Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)


crates/committer/Cargo.toml line 17 at r3 (raw file):

[dependencies]
starknet-types-core = "0.0.11"
starknet_api = "0.12.0-dev.0"

udeps should have failed here, as we don't use starknet-api since then. Investigating separately

Code quote:

starknet_api = "0.12.0-dev.0"

@dorimedini-starkware dorimedini-starkware force-pushed the dori/first-clap-usage-in-cli branch 2 times, most recently from ca662ed to e036c65 Compare April 8, 2024 19:25
Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @TzahiTaub)


crates/committer_cli/src/main.rs line 29 at r4 (raw file):

        /// The version of the class hash.
        #[clap(long)]
        class_hash_version: u8,

I think that it should accept it as a string that will later be converted to Felt.
PTAL here. Currently this is the input when hashing.


crates/committer_cli/src/main.rs line 61 at r4 (raw file):

}

#[derive(Debug, Args)]

please remove prefixes logic

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)


crates/committer_cli/src/main.rs line 29 at r4 (raw file):

Previously, nimrod-starkware wrote…

I think that it should accept it as a string that will later be converted to Felt.
PTAL here. Currently this is the input when hashing.

Done.


crates/committer_cli/src/main.rs line 61 at r4 (raw file):

Previously, nimrod-starkware wrote…

please remove prefixes logic

Done.

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1, 1 of 1 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware, @dorimedini-starkware, and @nimrod-starkware)


Cargo.toml line 17 at r6 (raw file):

[workspace.dependencies]
clap = { version = "4.5.4", features = ["cargo", "derive"] }
derive_more = "0.99.17"

Is this used?


crates/committer_cli/src/main.rs line 9 at r6 (raw file):

pub struct CommitterCliArgs {
    #[clap(flatten)]
    global_opts: GlobalOpts,

Needed to run the CLI to understand what this means...

Suggestion:

 global_options: GlobalOptions

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)


Cargo.toml line 17 at r6 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Is this used?

yes


crates/committer_cli/src/main.rs line 9 at r6 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Needed to run the CLI to understand what this means...

currently not in use; this is the struct where we will add options that are common to all commands.
if we see there is no such thing, we will delete this struct

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @nimrod-starkware)

Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware)

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Apr 11, 2024
Merged via the queue into main with commit 0714dfb Apr 11, 2024
9 of 10 checks passed
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.

4 participants