Fixes #58 - Auto increment for default name #469
Conversation
xprazak2
force-pushed
the
proj-name
branch
3 times, most recently
from
c873328
to
ba5b858
Compare
|
All tests are finally passing. Apologies it took me so long to make them green. |
|
hey @xprazak2 ! Sorry this has been sitting on the stack for awhile. I reviewed the code, and actually I don't think we want to cache the directory name every time wrangler runs; rather, we want to have Wrangler respond to an "already exists" OS error by reading the contents of the current working directory, and adding a number if the default name already exists, similar to how filenames are updated in a downloads folder when you download the same asset twice. So, if We really try to minimize the amount of local state Wrangler has to keep track of to keep our footprint relatively small. This does have a small disadvantage, wherein one could accidentally overwrite a worker because of naming collisions, but we would rather solve that with #330 . |
xprazak2
force-pushed
the
proj-name
branch
4 times, most recently
from
596d648
to
1f79afa
Compare
|
Thank you for your review, I have made changes as suggested - wrangler no longer keeps additional state and scans current dir for the next available name. |
There was a problem hiding this comment.
This gets most of the way there! however i think it is still a little "smart"er than we strictly need, and often "smart"er leads to unwanted side effects.
I'm happy to work with you on making some of these changes, or to pick this up and finish it off if you don't have the bandwidth to implement the changes. Either way you will be credited in the changelog, this work is very much appreciated!
src/commands/generate/mod.rs
Outdated
| @@ -33,14 +35,80 @@ pub fn run_generate(name: &str, template: &str) -> Result<(), failure::Error> { | |||
| let tool_name = "cargo-generate"; | |||
| let binary_path = install::install(tool_name, "ashleygwilliams")?.binary(tool_name)?; | |||
|
|
|||
| let args = ["generate", "--git", template, "--name", name, "--force"]; | |||
| let new_name = match generate_name(name) { | |||
There was a problem hiding this comment.
i'd like all this logic to live in the top level "generate" function; run generate should only be responsible for running the cargo generate command, it should not also be in charge of naming.
also, i would suggest an early return: first check should be "does a directory by this name already exist in the current working directory?" if it does not, we can skip updating the name. i think i would put that check directly in the generate function above the log::info on line 18.
| commands::run(command, &command_name) | ||
| } | ||
|
|
||
| fn generate_name(name: &str) -> Result<String, failure::Error> { |
There was a problem hiding this comment.
i think it is worth adding some comments here to establish the convention.
src/commands/generate/mod.rs
Outdated
|
|
||
| for entry in fs::read_dir(current_dir)? { | ||
| let entry = entry?; | ||
| entry_names.push(entry.file_name()); |
There was a problem hiding this comment.
this will return a value whether the entry in question is a file or a directory. we should only be worried about directory collisions.
src/commands/generate/mod.rs
Outdated
| Ok(entry_names) | ||
| } | ||
|
|
||
| fn construct_name(bare_name: &str, num: Option<usize>) -> String { |
There was a problem hiding this comment.
i think this function could likely be replaced by a call to format! given the other comments in this review.
src/commands/generate/mod.rs
Outdated
| let digits = detect_num(name); | ||
| let mut new_name = String::from(name); | ||
| let mut chars = new_name.chars().collect::<Vec<char>>(); | ||
| let split = chars.split_off(new_name.len() - digits); | ||
| let bare_name = chars.into_iter().collect::<String>(); | ||
|
|
||
| let mut num = match split.into_iter().collect::<String>().parse::<usize>() { | ||
| Ok(val) => Some(val), | ||
| Err(_) => None, | ||
| }; |
There was a problem hiding this comment.
I don't think we should try to split the given name this way. if it makes it easier to parse, we can use a - or _ separator for the digits.
try using a regex like /^name-\d+$/ against the directory names in the current working directory. if someone passes worker-2 and that already exists, we'll end up with worker-2-1, which is fine.
| Ok(new_name) | ||
| } | ||
|
|
||
| fn read_current_dir() -> Result<Vec<OsString>, failure::Error> { |
There was a problem hiding this comment.
i would also suggest passing the name in and only returning colliding directories.
|
hey @xprazak2 , just bumping this, sorry i forgot to mention you in my review last week. what do you think about these comments? if you have the bandwidth to implement that would be lovely, if not please let me know and i can pick this up and finish it up, you'll of course be credited in the changelog. thanks again for this PR, it is very much appreciated! |
|
I simplified the logic for generating a new name. The project name is no longer scanned for digits at the end but |
|
@ashleymichal, any chance you could do a re-review please? |
Fixes #58
Stores the name of the last default project
in global config and increments it
when user generates a project without a name.