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

resmgr: properly implement --overwrite, fix #690 #797

Merged
merged 6 commits into from
Dec 12, 2022
Merged

Conversation

kba
Copy link
Member

@kba kba commented Feb 13, 2022

Until now, the ocrd resmgr download --overwrite flag only had an effect if it was not set: Download would abort because the file already exists.

This PR adds an overwrite kwarg to the different methods handling download/copying/extraction. When set, the target directory/file are removed if they exist before data is written.

@kba kba requested a review from bertsky February 13, 2022 14:07
@kba kba linked an issue Feb 13, 2022 that may be closed by this pull request
Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to just rmtree(fpath) after resource_manager.download's L251 if fpath.exists() and not overwrite:?

@kba
Copy link
Member Author

kba commented Feb 14, 2022

Wouldn't it be simpler to just rmtree(fpath) after resource_manager.download's L251 if fpath.exists() and not overwrite:?

Simpler, yes, but I thought it would be less surprising if the overwrite case is handled by the download/copy method backends, because we have that pattern in other places too. You're right though, that all overwrite=True does is basically rm -rf dst before cp/download src dst.

@bertsky
Copy link
Collaborator

bertsky commented Feb 14, 2022

but I thought it would be less surprising if the overwrite case is handled by the download/copy method backends, because we have that pattern in other places too

I can only see downloading via requests in Resolver.download_to_directory, and in Workspace.download_file / download_url, which delegates to the former.

But even if you are planning on streamlining this into OcrdResourceManager.download in the future, it would still have to pass the top-level fpath.exists() test.

@kba kba self-assigned this Oct 25, 2022
@kba
Copy link
Member Author

kba commented Dec 12, 2022

But even if you are planning on streamlining this into OcrdResourceManager.download in the future, it would still have to pass the top-level fpath.exists() test.

9bc3aed Now it is done at the first fpath.exists() check, overwrite kwarg is unnecessarily convoluted.

@kba kba merged commit 9a89127 into master Dec 12, 2022
@kba kba deleted the resmgr-overwrite branch December 12, 2022 18:17
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

Successfully merging this pull request may close these issues.

resmgr: download --overwrite does not work
2 participants