-
-
Notifications
You must be signed in to change notification settings - Fork 169
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 force flag to allow user defined project name #69
Conversation
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.
Thanks!
I'm not really happy with --force
as a name, because it's not clear what it enforces. Overwrite the directory if it already exists? Don't error when the template is broken? We should try to come up with a more specific name, and also add some help texts to the CLI --help
stuff.
src/template.rs
Outdated
let mut template = liquid::Object::new(); | ||
template.insert( | ||
String::from("project-name"), | ||
liquid::Value::scalar(&name.kebab_case()), | ||
liquid::Value::scalar(&project_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.
FYI If project name is only used here you can also move it (scalar
would otherwise clone the string internally)
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.
Thank you for clarification! Project name is currently used only here, so I moved it!
src/main.rs
Outdated
@@ -55,6 +55,8 @@ pub struct Args { | |||
git: String, | |||
#[structopt(long = "name")] | |||
name: Option<String>, | |||
#[structopt(long = "force", short = "f")] | |||
force: bool, |
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.
Force? Force what? Force how?
Can you give this a more speaking name (like --exact-project-name
) and/or add a doc comment (which will show up in --help
)?
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 decided to name this flag as --exact-project-name
because I couldn't come up with more suitable name than you suggested, and also added help message.
tests/integration/basics.rs
Outdated
.stdout(predicates::str::contains("Done!").from_utf8()); | ||
|
||
assert!( | ||
dir.read("foobar-project/Cargo.toml") |
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.
Wait—this foobar-project
here is in kebab case. Should the dir name also be the "raw" version?
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.
Oops...I fixed to apply user defined name to project directory.
hey @toVersus thanks so much for this! seems like @killercup and i disagree about the name of the flag ;) i think i would still like to advocate assuming the flag was |
No problem! I know we should make careful decision about command line flags because it is hard to change its name even if someone complains of its unhandiness or unclearness. There are pros and cons for all choices, so I'm looking forward to see your conclusion :) |
hey @toVersus - thanks so much for your patience. after chatting with @killercup we think |
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.
this looks great! travis is failing because of cargo fmt weirdness so i'm gonna go ahead an merge instead of making you rebase in my fix for that stuff <3 thanks so much for your effort and patience!
Thanks 👍 |
close: #66