new flag: Enable renaming during file upload if duplicate exists #1453
Conversation
|
I also thought about just renaming when |
|
Thanks for this! I'd really like to see some tests here, though. It just occurred to me that we have no tests for conflicting files without any flags set nor did we have one for
I think we want an error here by default. It's safer.
I do like the enum idea. I'm fine breaking compatibility here. We have to make sure it's well pointed out in the CHANGELOG so people can quickly spot it. |
|
Thanks for the review, I'll push with the tests once I have a bit more free time. |
|
I have added the tests for the three conditions mentioned previously. I copied the tests from the |
|
@Atreyagaurav Looking great! What do you think about the enum thing that we discussed above? |
|
I added it as enum, and made the default be error. So only breaking change is I have the tests updated as well. And I also manually tested. |
|
Hey, I'd like to get this into the next release if possible. Could you rebase this and then it should be good to merge? |
|
Hi, it looks like a lot of things were changed while doing the multipart file upload. Would have been nice to merge it before that. I'll have to look into the conflicts a little more deeply and understand the new process as well, do you think you can wait like a week for it? |
| /// What to do if existing files with same name is present during file upload | ||
| /// | ||
| /// If you enable renaming files, the renaming will occur by | ||
| /// adding numerical suffix to the filename before the final | ||
| /// extension. For example file.txt will be uploaded as | ||
| /// file-1.txt, the number will be increased untill a available | ||
| /// slot is found. |
There was a problem hiding this comment.
| /// What to do if existing files with same name is present during file upload | |
| /// | |
| /// If you enable renaming files, the renaming will occur by | |
| /// adding numerical suffix to the filename before the final | |
| /// extension. For example file.txt will be uploaded as | |
| /// file-1.txt, the number will be increased untill a available | |
| /// slot is found. | |
| /// What to do if existing files with same name is present during file upload | |
| /// | |
| /// If you enable renaming files, the renaming will occur by | |
| /// adding numerical suffix to the filename before the final | |
| /// extension. For example file.txt will be uploaded as | |
| /// file-1.txt, the number will be increased until an available | |
| /// filename is found. |
| -R, --rename-duplicate | ||
| Enable renaming files during file upload if duplicate exists | ||
|
|
||
| The renaming will occur by adding numerical suffix to the filename before the final extension. For example file.txt will be uploaded as file-1.txt, the number will | ||
| be increased untill a available slot is found. | ||
|
|
||
| [env: RENAME_DUPLICATE_FILES=] |
There was a problem hiding this comment.
This needs to be updated as it's not synced with the text in the --help output.
| fn write_file_contents(path: PathBuf, filename: &str, contents: &str) -> PathBuf { | ||
| let file_path = path.join(filename); | ||
| let mut file = File::create(&file_path).unwrap(); | ||
| file.write_all(contents.as_bytes()) | ||
| .expect("Couldn't write file"); | ||
| file_path | ||
| } |
There was a problem hiding this comment.
Do we really need this function and can't we use std::fs::write() instead?
| let test_file_name = "duplicate test file.txt"; | ||
| let test_file_contents = "Test File Contents"; | ||
|
|
||
| // create the file | ||
| let test_file_path = write_file_contents( | ||
| server.path().to_path_buf(), | ||
| test_file_name, | ||
| test_file_contents, | ||
| ); | ||
|
|
||
| // Before uploading, make sure the file is there. | ||
| let body = reqwest::blocking::get(server.url())?.error_for_status()?; | ||
| let parsed = Document::from_read(body)?; | ||
| assert!(parsed.find(Text).any(|x| x.text() == test_file_name)); |
There was a problem hiding this comment.
I think you can get rid of all of this in every test because you can use fixtures::FILES and try to overwrite them. They already exist and this should allow you to cut the tests by a lot. We use fixtures::FILES in many places in the test suite.
Yeah, sorry about that. Life happened. Anyway, I can certainly wait a week. :) |
Fixes #1452
I think this will work as a flag to rename files. It won't work when the file contains multiple extension like
.tar.gzwhich is hard to do, as we could make it work with that, but any file with multiple.in the text for no meaning will mess it up. But I can make that change if you think that's acceptable.Also, it will break compatibility, but we could combine the overwrite and rename flags into a single flag like
--duplicate-filesthat takes enum (rename/overwrite).