Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

[token-upgrade] Remove clap deprecated functions #6951

Merged

Conversation

samkim-crypto
Copy link
Contributor

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

Problem

The token-upgrade 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::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.
  • Replaced try_pubkey_of with SignerSource::try_get_pubkey. The try_pubkey_of internally tries to parse Pubkey from SignerSource, which unfortunately, solana-clap-v3-utils does not yet support. The SignerSource::try_get_pubkey parses SignerSource correctly and then extracts Pubkey from it.

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 the logic to parse signers directly.

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 other crates in the repo still uses some deprecated functions, so I removed this feature from clap for now.

This is an analogous PR to #6952.

@samkim-crypto samkim-crypto marked this pull request as ready for review July 3, 2024 05:13
@samkim-crypto samkim-crypto requested a review from joncinque July 3, 2024 05:13
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Just the one question, but the rest looks good to me!

@@ -336,7 +334,7 @@ async fn main() -> Result<(), Box<dyn Error>> {
.value_parser(SignerSourceParserBuilder::default().allow_all().build())
.value_name("MULTISIG_SIGNER")
.takes_value(true)
.multiple(true)
.action(ArgAction::Append)
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation seems to imply that we should use multiple_occurrences instead, ie:

Arg::multiple_occurrences in favor of Arg::multiple_values (positional), ArgAction::Append (flag with value), or ArgAction::Count (flag without value)

from clap-rs/clap#3797, and:

Replace Arg::multiple(bool) with Arg::multiple_values / Arg::multiple_occurrences

from https://github.com/clap-rs/clap/releases/tag/v3.0.0

Is there a reason to use your approach instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah multiple was deprecated in favor of multiple_occurrences in v3.0.0, but multiple_occurrences was deprecated in favor of ArgAction in v3.2.0 (https://github.com/clap-rs/clap/releases/tag/v3.2.0) 😅.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha I had a feeling, thanks for the info!

@samkim-crypto samkim-crypto merged commit 7d24772 into solana-labs:master Jul 16, 2024
8 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants