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

Add warnings while using common arguments #2573

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

kkawula
Copy link
Collaborator

@kkawula kkawula commented Oct 8, 2024

Relates #2552

Introduced changes

  • Added displaying warning while using common arguments

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@kkawula
Copy link
Collaborator Author

kkawula commented Oct 10, 2024

It generates such output:
image

@kkawula kkawula marked this pull request as ready for review October 10, 2024 12:38
crates/sncast/src/main.rs Outdated Show resolved Hide resolved
crates/sncast/src/main.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

For this to make sense, it should also already be possible to pass these arguments in the subcommands we are relocating them to. Because right now, it's impossible to avoid having these warnings displayed.

What I'd like is if for example someone does this

sncast --account my_account invoke ...

It should show a warning, but it should already be possible to do

sncast invoke --account my_account

And not get shown a warning.

@kkawula kkawula requested a review from cptartur October 14, 2024 11:42
@@ -66,21 +67,21 @@ Report bugs: https://github.com/foundry-rs/starknet-foundry/issues/new/choose\
#[allow(clippy::struct_excessive_bools)]
struct Cli {
/// Profile name in snfoundry.toml config file
#[clap(short, long)]
#[clap(short, long, global = true)]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making these arguments global, let's actually add them as arguments to subcommands that use them. And keep them as global as well to retain current cli.

If a user uses the global version show a warning but continue program execution.

If both are provided, throw an error.

Copy link
Member

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Tip: I think could use this https://docs.rs/clap/4.5.20/clap/struct.Arg.html#method.value_parser, log a waring in custom parser and then parse with standard clap parser https://docs.rs/clap/4.5.20/clap/macro.value_parser.html
(thought about it for whole 30 s, so don't take it as granted)

@kkawula kkawula marked this pull request as draft October 27, 2024 20:35
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.

4 participants