-
Notifications
You must be signed in to change notification settings - Fork 125
fix: replace deprecated #[clap(...)] with #[command(...)] and #[arg #3224
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
base: main
Are you sure you want to change the base?
Conversation
@sveitser Could you check this ? There are no conflicts now. |
@@ -20,40 +20,40 @@ use vbs::version::StaticVersionType; | |||
#[derive(Clone, Debug, Parser)] | |||
struct Options { | |||
/// Start counting from block FROM. | |||
#[clap(long, name = "FROM")] |
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.
Why change name
to id
?
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.
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.
Shouldn't we use value_name
then?
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.
@sveitser Yes, you're right. I updated name
to value_name
as you suggested.
abc9968
to
61e2cf7
Compare
@sveitser Could you run CI again ? I force pushed branch for fmt. |
61e2cf7
to
6a41254
Compare
Please never force push to a branch after having received a review. We need to review everything again. |
Also there is more to fix on your branch now, since code has been added recently:
Why not enable the deprecated feature by default so everyone gets warnings if they use deprecated features? At least as |
Hi @sveitser , I will force pushed (because so mony conflicts here.) branch. After that i can open other PR to solve if you accepted. |
10ebfae
to
877a7c4
Compare
877a7c4
to
031a09e
Compare
f0d85cb
to
8d61889
Compare
Hi @sveitser, It's ready for review now. - name: Check if deprecated clap feature triggers warnings
run: |
cargo clippy --workspace --all-targets --no-default-features --features clap/deprecated \
-- -D warnings |
8d61889
to
71de056
Compare
@sveitser Could you check again ? |
@sveitser Could you check ? |
.github/workflows/lint.yml
Outdated
@@ -45,6 +45,11 @@ jobs: | |||
cargo clippy --workspace --features testing --all-targets --keep-going \ | |||
-- -D warnings | |||
|
|||
- name: Check if deprecated clap feature triggers warnings |
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.
I don't think it's necessary to run clippy twice with different features, please remove this.
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.
Done sir.
@@ -4509,7 +4509,7 @@ pub mod LightClientArbitrumV2 { | |||
#[rustfmt::skip] | |||
#[allow(clippy::all)] | |||
pub static BYTECODE: alloy_sol_types::private::Bytes = alloy_sol_types::private::Bytes::from_static( |
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.
Your PR unexpectedly changes the contract bytecode.
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.
I believe this change isn't part of my PR — I haven’t touched lightclientarbitrumv2.rs, and it also doesn’t appear under “Files changed”. Most likely, this was introduced by a previous merge from main.
@sveitser Ready for review sir. |
Closes #2826
Ref : #2827 (comment)
This PR:
-Replaces deprecated
#[clap(...)]
attributes with the updated#[arg(...)]
,#[command(...)]
, and#[arg(value_name = ...)]
formats throughout the codebase.-Cleans up redundant/default attributes such as:
#[arg(action)]
→ removed, since action is now the default in clap v4#[arg(value_parser)]
→ removed, also default nowReplaces all
#[arg(name = "...")]
with#[arg(value_name = "...")]
as per current best practicesThis PR does not:
-Introduce any logic changes beyond the attribute replacements
-Refactor surrounding CLI parsing logic
Key places to review:
-All CLI argument definitions using
#[arg(...)]
-Replacements of
name = "...“
withvalue_name = "..."
-Removed empty or default
action
andvalue_parser
usagesThings tested
cargo check
cargo check --features clap/deprecated
cargo build
✅ All tests passed successfully.