Skip to content
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

download deletes folder when using a folder name in parameter output #198

Open
franzfurbass opened this issue Jun 9, 2022 · 4 comments
Open

Comments

@franzfurbass
Copy link

The function download assumes that the output parameter (of type string) is a file. When given a folder name as output parameter the whole folder is deleted which is unexpected.
e.g. download(url_to_file, ".") removes the current directory. Compared to copy the expected behavior is to put the file into the directory like in cp path_to_file/file.txt ..

The output parameter should be checked with isdir and in that case the a filename derived from the folder name should be used instead.

@StefanKarpinski
Copy link
Member

I agree that downloading to . should definitely not delete the current directory. However, I'm not convinced that it's good for non-interactive programming usage that the behavior of download(url, "path") depends on the state of the file system. I think it should either save the content of url to "path" or fail. It might make sense to add a force::Bool=false option that controls whether the output path is overwritten or not. It might also make sense to have an into::Bool=false option that causes the download to be saved into the target rather than at the target. The error message when someone tries download(url, "dir") and it fails could suggest using into=true to save into the directory or force=true to replace the directory.

@StefanKarpinski
Copy link
Member

Technically adding a force option defaulting to false is breaking, but I think we could do it in a minor release. It seems unlikely that people are relying on this deleting directories.

@ararslan
Copy link
Member

ararslan commented Jun 9, 2022

That would break my workflow of using the shorter and clearer download(url, "dir") in place of rm("dir"; recursive=true).

@StefanKarpinski
Copy link
Member

:trollface:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants