-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[token-cli] Upgrade to clap-v3 #6376
[token-cli] Upgrade to clap-v3 #6376
Conversation
3da1fc2
to
a863f08
Compare
a863f08
to
86a5aef
Compare
e2d57ff
to
07450df
Compare
i took a first read through and it seems pretty straightforward. i was wondering about the removal of the |
Thanks for taking a look! and sorry the details were a bit terse with the issue surrounding The issue with Clap-v2 did not really complain about this (the alias was just useless). But clap-v3 is smart enough to catch this and gives an error when invoked, which is why I removed the alias. This should not break existing commands because it was a useless alias from the start. |
This comment was marked as spam.
This comment was marked as spam.
07450df
to
23a827d
Compare
23a827d
to
f420049
Compare
token/cli/src/clap_app.rs
Outdated
.validator(is_amount) | ||
.value_parser(clap::value_parser!(f64)) |
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.
can this parse a number without a decimal?
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.
Yes you are completely right! The validation is_amount
should actually be replaced with using the Amount
type, but I thought making this change in this PR would add on a whole new set of changes, so I just tried using the built-in clap value parser without thinking through it carefully. Thanks for the catch!
I added the is_amount
validation back in and updated the processor logic to parse as string first and then convert it to f64
since value_of
is deprecated. On a subsequent PR, I will replace the whole validator with Amount
so that we can parse more ergonomically/cleanly instead of parsing as string.
token/cli/src/clap_app.rs
Outdated
.validator(is_amount) | ||
.value_parser(clap::value_parser!(f64)) |
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.
same here, not sure if these are supposed to stay is_smount
fd6ee0e: Upgrade to clap version
3.2.23
and change dependencysolana-clap-utils
-->solana-clap-v3-utils
145285f, 515ddc4, ad1fa87: Lifetimes were removed from
ArgMatches
,Arg
, andApp
with clap-v3 (clap-rs/clap#4223, clap-rs/clap#2150, clap-rs/clap#1041), so they were removed.e6f43c6:
min_values
andmax_values
now takeusize
instead ofu64
, so I updated these inputs.7f40b38:
short
now takeschar
instead of string, so I updated these inputs.97b288c: The requirement on validator functions have been updated from
F: Fn(String) -> ...
toF: FnMut(&str) -> ...
, so I updated the syntax for custom validator functions.a7c5773:
ArgMatches::subcommand
now returnsOption<(&str, &ArgMatches)>
instead of(&str, Option<&ArgMatches>)
, so I updated these.79e11b6:
possible_values
is now more generic and does not automatically infer a string type for its inputs, so I updated to specifically cast to string.d2a1271: Some validation functions have been deprecated in clap-v3-utils. However, removing these deprecated functions will require another set of large changes, so I added
#![allow(deprecated)]
for now. I created #6393 for this and will update in a subsequent PR.cb8cc28: This part is a little bit tricky, but the validator syntax changed from clap-v2 to clap-v3.
Fn(String) -> Result<...>
FnMut(&str) -> Result<...>
The existing custom validator functions have the following syntax:
In clap-v2, adding
Arg::validator(custom_validate)
was not a problem because the genericT
could be inferred to beString
. However, in clap-v3, the genericT
must be inferred to be&str
and since&str
is a borrowed type,&'a str
for some fixed lifetime'a
. However,Arg::validator
requirescustom_validate
to be of any lifetime, which causes rust to complain. It seems like adding a level of indirection by wrapping the custom validator function inside a closure allows the lifetime associated withT: &'a str
to be flexibly chosen on the spot is a workaround this, so I made these changes in this commit.0daf1a9: Clap-v3 requires positional parameters either all or none of the arguments, so I added indices to some arguments.
ff00bfd: There is currently a useless alias
owner
for specifyingKEYPAIR
s in some subcommands. If the aliasowner
is used to specify a keypair, the argument is interpreted asOWNER_ADDRESS
(fromowner_address_arg()
), so it can never really be invoked. I removed this alias since clap-v3 complains.0afb444: In clap-v3, positional arguments can't have long or short names (which makes sense!), so I removed these.
b613066: In clap v2,
value_of
andis_present
returnsNone
if the argument id is not previously specified inArg
. In contract, in clap v3,value_of
panics if the argument id is not specified previously asArg
(see solana-labs/solana#33184). I added custom logic to account for this difference in versions. In some places, these changes are a bit clunky, but cleaning them up will require larger changes, so I will do it in subsequent PRs.ac068bf: This commit is related to the previous commit. The functions
signer_from_path
andsigner_from_path_with_config
insolana-clap-v3-utils
panics early in a special case that we cannot easily account for from the caller side. I added custom versions of these functions that specifically checks for this special case.a863f08:
cmd.args([...])
mutatescmd
to include the specified arguments. Currently,cmd.args(args).assert()..stderr("error: Could not find config file
~/nonexistent/config.yml\n");
andcmd.assert().code(1).failure();
addsargs
tocmd
twice, so I fixed this.