-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Allow Cargo new
on existing empty dirs
#781
Conversation
Cargo new
on existing empty dirs
@@ -23,9 +23,12 @@ struct CargoNewConfig { | |||
} | |||
|
|||
pub fn new(opts: NewOptions, _shell: &mut MultiShell) -> CargoResult<()> { | |||
fn dir_is_empty(p: &Path) { fs::walk_dir(p).unwrap().count() == 0 } |
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.
Is unwrap
ok here? It doesn't look good, but I can't find a better alternative
@@ -23,9 +23,12 @@ struct CargoNewConfig { | |||
} | |||
|
|||
pub fn new(opts: NewOptions, _shell: &mut MultiShell) -> CargoResult<()> { | |||
fn dir_is_empty(p: &Path) -> bool { fs::walk_dir(p).unwrap().count() == 0 } |
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.
Is unwrap
ok here? It doesn't look good, but I can't find a better alternative
Could you expand a little on the motivation for this? It may be a little too surprising to some that |
I just thought this would be useful after having the following issue:
I think this is pretty surprising. With this PR I want to at least allow the second option. Checking that the directory is not empty is just an easy way of ensuring no files will be overwritten, but it could also be done in a more fine-grained way. |
FWIW, I ran into this a couple times when starting with Cargo, since I tried |
Closing for now as I'd like to flesh out the |
No description provided.