-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add "sync" option for upload & download #616
Conversation
Codecov Report
@@ Coverage Diff @@
## master #616 +/- ##
==========================================
+ Coverage 84.10% 84.55% +0.45%
==========================================
Files 59 59
Lines 5717 5932 +215
==========================================
+ Hits 4808 5016 +208
- Misses 909 916 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
From code review alone looks fine, yet to try. Thank you @jwodder. Left minor suggestion and filed a dedicated issue about allowing for non-interactive use.
dandi/download.py
Outdated
a_path = a_path.replace("\\", "/") | ||
if a_path not in asset_paths: | ||
to_delete.append(p) | ||
if to_delete and click.confirm(f"Delete {len(to_delete)} local assets?"): |
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.
if to_delete and click.confirm(f"Delete {len(to_delete)} local assets?"): | |
if to_delete and click.confirm(f"Delete {len(to_delete)} local asset{'s' if len(to_delete) > 1 else ''}?"): |
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.
Let's improve UX right away, and instead of plain confirm give a choice for "[l]ist" or alike and if asked, list all the assets to be deleted so user could make an informed decision
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.
filed related #620 on allowing for non-interactive, which might be done after or melded into this PR if we agree before it is ready
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.
Done.
Didn't try but looks great. Thank you @jwodder , let's proceed |
Closes #365.
Does not include renaming.