Skip to content
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

[transfer-hook] Remove clap deprecated functions #6952

Merged
merged 6 commits into from
Jul 2, 2024

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented Jun 30, 2024

Problem

The transfer hook cli tool still uses some deprecated functions from clap-v2.

Summary

Updated the deprecated functions from clap-v2. Most of the changes should be straightforward:

  • Replaced Arg::with_name to Arg::new, which is functionally identical
  • Replaced Arg::multiple with ArgAction::Append, which should be functionally identical
  • Replaced is_present with try_contains_id. The old is_present panics if the arg is not previously known/defined while try_contains_id gives an error. Otherwise, the two functions are identical.

The only non-trivial change would be replacing value_of. This should technically be updated to try_get_one::<T>(...) or get_one::<T>(...), but I replaced it with a convenience function try_get_signer, which was added with solana-clap-v3-utils 2.0. I ended up removing the use of DefaultSigner and just added logic to parse signers directly.

I also used SignerSource::try_get_pubkey to remove some verbose parsing code (from #6625).

The feature deprecated in clap v3 allows us to toggle warnings from the use of deprecated functions. The deprecated functions are all removed from this crate, but the other crates in the repo still uses some deprecated functions, so I removed this feature from clap for now.

@samkim-crypto samkim-crypto marked this pull request as ready for review July 2, 2024 03:50
@samkim-crypto samkim-crypto requested a review from buffalojoec July 2, 2024 07:22
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Looks good!

@samkim-crypto samkim-crypto merged commit 8166a1d into solana-labs:master Jul 2, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants