Conversation
|
How about checking that if the output exists, fail immediately, ie. don't clobber would be the default? |
|
@langma considering that the purpose of the download command is to get the newest config, i would argue that failing if a file with the same name already exists in the folder would be totally counter productive. And i mean this far we have been very much ok with nuking the whole damn directory, so i really don't see the need to now go all tippy-toes 😅 |
|
The problem with the approach in this PR is that if you give the wrong output directory, for example another app's config dir, some of the files are overwritten while others remain. The result would be incorrect for both of the apps. Failing immediately would at least keep the data intact. |
|
How is that worse from nuking the whole directory? |
langma
left a comment
There was a problem hiding this comment.
If the app contains multiple lookup files instead of a single config.yaml, mixed download introduces the possibility to leave unused files behind while overwriting some anyway. I'm not sure which one is worse, but they are both not ideal. I'm trying to figure out a way to make the default operation both understandable and respectful of the user's local files.
There are three possible scenarios:
- given output does not exist: create it and download files
- there is an empty directory: download files
- there are any number of files in the output directory: try to do the right thing...
The third is basically a merge operation, which are not trivial to do properly. The current operation is the "accept-theirs" strategy, where your local changes are discarded. This PR does the same, but leaves any non-related files alone, which is already better than before. It still makes an automatic merge though, which might be good but might also be a mistake, especially if the output directory is not what the user meant.
As the outcome is still IMO better than before, I'll approve this, but what I was trying to go after was a change to the merge responsibility: if there are files, make the user acknowledge
that they will be overwritten. Easiest would be a --force flag, which requires no merge logic to be implemented.
|
I'll merge this now. Maybe for future let's add a prompt (y/n) if it is about to overwrite. |
Previously when invoking the
downloadcommand, it would clear the whole directory and then write the downloaded configuration in that folder. This was a particularly nasty if one wanted to download theconfig.yamlin the current working directory. Personally i think this behaviour is rather dangerous, as the user might accidentally loose any unsaved/committed work.In this PR the clearing of the download directory is removed. Any files in the download directory with the same filename as in the configuration will be overwritten, though. All other files in the download directory are left as is.