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

Support specify contract path input with or without -p flag #361

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ndkazu
Copy link

@ndkazu ndkazu commented Dec 3, 2024

Description

This PR is addressing issue #350.
Allow pop-cli to supports two ways to to specify the path to a contract or artifact file: using the -p flag or specifying the path directly without it:

pop call contract -p ../contract.json 

and

pop call contract ../contract.json

should both be possible.

Implementation

Two arguments are used to specify the path: One Optional, and one positional:

#[arg(long,required = false)]
	pub(crate) path: Option<PathBuf>,
	#[arg(value_name = "PATH",required = false)]
	pub(crate) path1: Option<PathBuf>,

The following contract-related commands will be upgraded

  • pop build
  • pop up contract
  • pop call contract

@AlexD10S
Copy link
Collaborator

AlexD10S commented Dec 4, 2024

Thanks for your interest in contributing to pop-cli!
I’ve reviewed your approach, and maintaining a second argument path1 might not be the cleanest way to solve this.
You might want to explore using positional arguments, as discussed here clap-rs/clap#2260, or consider the use of subcommands (https://docs.rs/clap/latest/clap/trait.Subcommand.html) instead.

@ndkazu
Copy link
Author

ndkazu commented Dec 6, 2024

Thanks for your interest in contributing to pop-cli! I’ve reviewed your approach, and maintaining a second argument path1 might not be the cleanest way to solve this. You might want to explore using positional arguments, as discussed here clap-rs/clap#2260, or consider the use of subcommands (https://docs.rs/clap/latest/clap/trait.Subcommand.html) instead.

Hello, I looked into it, and it seems to me that what we're trying to do is closer to this, and this.
and from what I understood from these discussions involving the Clap devTeam, it seems like using two arguments is the way to go(maybe not the way I did it though... maybe use required_unless = ... instead of required = ... ). An experimental module exists here, but I don't think the CLAP team is wiling to consider it.

@ndkazu
Copy link
Author

ndkazu commented Dec 7, 2024

@AlexD10S , I made a cleaner version of my first suggestion (modifications are minimal, and limited to pop build ...). I am still looking for other (less invasive?) routes, but based on my previous comment, let me know if it makes sense to propagate this method to pop up contract & pop call contract.

@AlexD10S
Copy link
Collaborator

@AlexD10S , I made a cleaner version of my first suggestion (modifications are minimal, and limited to pop build ...). I am still looking for other (less invasive?) routes, but based on my previous comment, let me know if it makes sense to propagate this method to pop up contract & pop call contract.

Thanks! Running the CI to check all tests pass, and I'll do a proper review

@ndkazu
Copy link
Author

ndkazu commented Dec 14, 2024

@AlexD10S , I made a cleaner version of my first suggestion (modifications are minimal, and limited to pop build ...). I am still looking for other (less invasive?) routes, but based on my previous comment, let me know if it makes sense to propagate this method to pop up contract & pop call contract.

Thanks! Running the CI to check all tests pass, and I'll do a proper review

I saw that the CLI was failing, and did a cargo +nightly fmt as a tentative to fix the error.

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