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
Upgraded clap to v4 #3209
Upgraded clap to v4 #3209
Conversation
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.
Looking good overall. Please, fix the formatting and use the owned API.
@@ -227,7 +227,8 @@ pub trait Handler<M>: Actor { | |||
|
|||
#[async_trait::async_trait] | |||
impl<H, M> DeferableReplyHandler<M> for H | |||
where H: Handler<M> | |||
where |
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.
Use make fmt
to format your changes according to our code style. It requires the nightly
toolchain.
quickwit/quickwit-cli/src/index.rs
Outdated
@@ -261,14 +261,16 @@ impl IndexCliCommand { | |||
|
|||
fn parse_clear_args(matches: &ArgMatches) -> anyhow::Result<Self> { |
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.
With new API, it makes more sense to pass owned ArgMatches
(see remove_subcommand
) so we can use the remove_one
, try_remove_one
function in subcommand parsers.
quickwit/quickwit-cli/src/index.rs
Outdated
@@ -261,14 +261,16 @@ impl IndexCliCommand { | |||
|
|||
fn parse_clear_args(matches: &ArgMatches) -> anyhow::Result<Self> { | |||
let cluster_endpoint = matches | |||
.value_of("endpoint") | |||
.get_one::<String>("endpoint") |
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.
This would become:
.remove_one::<String>("endpoint").map(Url::from_str).expect("...")?;
.map(Url::from_str) | ||
.expect("`endpoint` is a required arg.")?; | ||
let index_id = matches | ||
.value_of("index") |
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.
This would become:
remove_one::<String>("index").expect("...");
quickwit/quickwit-cli/src/tool.rs
Outdated
.expect("`config` is a required arg.")?; | ||
let target_dir = matches | ||
.value_of("target-dir") | ||
.get_one::<String>("target-dir") |
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.
You can get_one::<PathBuf>
or remove_one::<PathBuf>
directly.
I'm going to merge this PR on a feature branch and take over so we can land the upgrade before the next release. Thanks @gautamprikshit1! |
Description
Describe the proposed changes made in this PR.
Upgraded clap to v4.
Fixes #2236