-
Notifications
You must be signed in to change notification settings - Fork 66
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
Rework contract init #1625
base: init-deprecate
Are you sure you want to change the base?
Rework contract init #1625
Conversation
272aa0e
to
840af02
Compare
Initialize a Soroban project with an example contract | ||
Initialize a Soroban contract. | ||
|
||
When running with empty or non-existent `--project-path`, this command will generate a template Cargo workspace project and add a sample contract package. When running in the existing Cargo project, it will add a new package for a sample contract with a given `--name`. |
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.
isn't the project path positional? You don't actually have --project-path
.
use crate::commands::contract::init::Error::{ | ||
AlreadyExists, PathExistsNotCargoProject, PathExistsNotDir, | ||
}; | ||
use crate::{commands::global, print}; | ||
|
||
#[derive(Parser, Debug, Clone)] | ||
#[group(skip)] | ||
pub struct Cmd { | ||
pub project_path: String, |
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 think this needs to be updated to Option<project_path>
. This will require either project_path
or --name
to be provided; if you run the command like stellar contract init
, it should error.
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.
Isn't project path always required? Name is just specifying the name of the contract in the workspace, but presumably we still need to tell the tool where the workspace is.
default_value = "", | ||
long_help = "An optional flag to pass in a url for a frontend template repository." | ||
action = clap::ArgAction::HelpLong, | ||
long_help = "This argument has been deprecated and will be removed in the future versions of CLI. You can still clone examples from the repo https://github.com/stellar/soroban-examples", |
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.
Given that these deprecated switches are in fact noop, I think we should mention that the switch doesn't do anything anymore. The way is phrased right now gives the impression it worked, but won't in the future.
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.
If the switches don't do anything anymore, we should remove them so folks get a hard error if they try to use it.
if project_path.exists() { | ||
return Err(AlreadyExists(self.args.name.clone())); | ||
} |
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 think we should keep allowing users to overwrite a project. It's a great way to upgrade all your project, because you can undo undesired changes with git, while keeping everything else.
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.
The behavior has changed, so I don't think this flag is needed anymore
840af02
to
fff708a
Compare
fff708a
to
e294872
Compare
I'm converting this PR back to draft, I think my original implementation is not what we would like to move forward with |
2298378
to
2ea8dee
Compare
What
Rework
contract init
. Not it does:Why
Changes according to #1586
Known limitations
--overwrite
support has been removed. If we decide to keep it in #1628, this PR should return it back