Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Clap v3 update: keygen #24479

Merged
merged 5 commits into from
Apr 22, 2022
Merged

Conversation

KirillLykov
Copy link
Contributor

Problem

Update clap to version 3. This PR is part of the effort to migrate the whole repo to clap-v3

Summary of Changes

  • update CLI to use clap v3
  • switch from clap-utils to clap-v3-utils

@KirillLykov KirillLykov changed the title Clap3 up keygen Clap v3 update: keygen Apr 19, 2022
@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #24479 (f15dbe8) into master (3155270) will increase coverage by 11.8%.
The diff coverage is 67.1%.

❗ Current head f15dbe8 differs from pull request most recent head cd87e5d. Consider uploading reports for the commit cd87e5d to get more accurate results

@@             Coverage Diff             @@
##           master   #24479       +/-   ##
===========================================
+ Coverage    70.0%    81.9%    +11.8%     
===========================================
  Files          36      592      +556     
  Lines        2255   163645   +161390     
  Branches      322        0      -322     
===========================================
+ Hits         1580   134102   +132522     
- Misses        560    29543    +28983     
+ Partials      115        0      -115     

.validator(grind_validator_starts_with)
.multiple_occurrences(true)
.multiple_values(true)
.validator(|s| grind_validator_starts_with(s.to_string()))
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't look like grind_validator_starts_with strictly requires a String. I'd prefer to change its signature to take &str

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing out, fixed all the occurrences except for one which is more elaborated

.validator(grind_validator_ends_with)
.multiple_occurrences(true)
.multiple_values(true)
.validator(|s| grind_validator_ends_with(s.to_string()))
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

.long("num-threads")
.value_name("NUMBER")
.takes_value(true)
.validator(is_parsable::<usize>)
.validator(|s| is_parsable::<usize>(s.to_string()))
Copy link
Contributor

Choose a reason for hiding this comment

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

what's constraining the v3 version of is_parsable to taking a String?

.index(1)
.value_name("KEYPAIR")
.takes_value(true)
.validator(is_prompt_signer_source)
.validator(|s| is_prompt_signer_source(s))
Copy link
Contributor

Choose a reason for hiding this comment

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

superfluous closure?

Copy link
Contributor Author

@KirillLykov KirillLykov Apr 20, 2022

Choose a reason for hiding this comment

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

without this closure it doesn't compile with error:

error: implementation of `FnOnce` is not general enough
   --> keygen/src/keygen.rs:500:26
    |
500 |                         .validator(is_prompt_signer_source)
    |                          ^^^^^^^^^ implementation of `FnOnce` is not general enough
    |
    = note: `fn(&'2 str) -> Result<(), std::string::String> {is_prompt_signer_source::<&'2 str>}` must implement `FnOnce<(&'1 str,)>`, for any lifetime `'1`...
    = note: ...but it actually implements `FnOnce<(&'2 str,)>`, for some specific lifetime `'2`

error: could not compile `solana-keygen` due to previous error

I found explanation here: "The issue stems from how closures, type inference, and higher-order signatures play together: for historical reasons, closures favor type inference / are reluctant to get higher-order promoted. In fact, they "rarely" do, except for one special hard-coded case ... "
Looks like it's hard to do something with it

Copy link
Contributor

Choose a reason for hiding this comment

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

even when adapting the signature of is_prompt_signer_source to explicitly take &str? I'd think it's just not able to resolve AsRef<str> to &str automatically 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I use &str it works (updated). Looks like validator is always assuming &str so might be unnecessary to have generics there. My only concern is that all the functions in the input_validators.rs are using the same generic signature and may be there is a reason behind which is unknown to me. So I got rid of generic only in is_prompt_signer_source, others I left as they are to modify in next PRs by demand, if makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool! agreed updating all of the validators in a future PR is the right thing

@KirillLykov KirillLykov requested a review from t-nelson April 20, 2022 12:55
@KirillLykov KirillLykov self-assigned this Apr 21, 2022
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm. thanks!

@KirillLykov KirillLykov merged commit a221014 into solana-labs:master Apr 22, 2022
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Apr 22, 2022
* update clap to v3: keygen

* use clap-v3-utils

* update Cargo.lock

* address PR comments

* get rid of unnecessary generic for clap validator
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
* update clap to v3: keygen

* use clap-v3-utils

* update Cargo.lock

* address PR comments

* get rid of unnecessary generic for clap validator
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 30, 2022
* update clap to v3: keygen

* use clap-v3-utils

* update Cargo.lock

* address PR comments

* get rid of unnecessary generic for clap validator
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants