-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Conversation
Codecov Report
@@ 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 |
keygen/src/keygen.rs
Outdated
.validator(grind_validator_starts_with) | ||
.multiple_occurrences(true) | ||
.multiple_values(true) | ||
.validator(|s| grind_validator_starts_with(s.to_string())) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
keygen/src/keygen.rs
Outdated
.validator(grind_validator_ends_with) | ||
.multiple_occurrences(true) | ||
.multiple_values(true) | ||
.validator(|s| grind_validator_ends_with(s.to_string())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
keygen/src/keygen.rs
Outdated
.long("num-threads") | ||
.value_name("NUMBER") | ||
.takes_value(true) | ||
.validator(is_parsable::<usize>) | ||
.validator(|s| is_parsable::<usize>(s.to_string())) |
There was a problem hiding this comment.
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
?
keygen/src/keygen.rs
Outdated
.index(1) | ||
.value_name("KEYPAIR") | ||
.takes_value(true) | ||
.validator(is_prompt_signer_source) | ||
.validator(|s| is_prompt_signer_source(s)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superfluous closure?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
95644ac
to
211af8a
Compare
b8dcacd
to
cd87e5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. thanks!
* update clap to v3: keygen * use clap-v3-utils * update Cargo.lock * address PR comments * get rid of unnecessary generic for clap validator
* update clap to v3: keygen * use clap-v3-utils * update Cargo.lock * address PR comments * get rid of unnecessary generic for clap validator
* update clap to v3: keygen * use clap-v3-utils * update Cargo.lock * address PR comments * get rid of unnecessary generic for clap validator
Problem
Update clap to version 3. This PR is part of the effort to migrate the whole repo to clap-v3
Summary of Changes
clap-utils
toclap-v3-utils