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

Make datasets.mv do nothing if source and destination are the same #162

Merged
merged 1 commit into from
May 4, 2020

Conversation

liamcoatman
Copy link
Contributor

Make datasets.mv do nothing if source and destination are the same.

Because mv is implemented as cp then rm, the previous behaviour was to delete the file if the source and destination are the same. The new behaviour is to do nothing (like the Unix mv command).

Copy link
Member

@imrehg imrehg left a comment

Choose a reason for hiding this comment

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

Good catch!

@acroz
Copy link
Member

acroz commented May 1, 2020

I suggest we close this in favour of #163. Then, it's the backend's responsibility to get this behaviour right. If the backend implements this behaviour wrongly, we should fix it. Thoughts?

@liamcoatman
Copy link
Contributor Author

I suggest we close this in favour of #163. Then, it's the backend's responsibility to get this behaviour right. If the backend implements this behaviour wrongly, we should fix it. Thoughts?

Agreed.

@liamcoatman liamcoatman closed this May 1, 2020
@jkeelan
Copy link

jkeelan commented May 2, 2020

I disagree, I think we should push this, because it prevents an unexpected and potentially destructive operation. To be clear, #163 also suffers from this issue, so it would have to be fixed on the backend, which will mean a further delay. While solving it on the front end isn't ideal, it's better than nothing.

@acroz
Copy link
Member

acroz commented May 4, 2020

Fair enough - let's merge this and deal with the backend implementation separately.

@acroz acroz reopened this May 4, 2020
@acroz acroz self-requested a review May 4, 2020 12:27
@acroz acroz force-pushed the datasets-mv-fix branch from cec1cd9 to 659352e Compare May 4, 2020 12:29
@acroz acroz merged commit 848d459 into master May 4, 2020
@acroz acroz deleted the datasets-mv-fix branch May 4, 2020 13:17
@acroz
Copy link
Member

acroz commented May 4, 2020

Released with 0.26.2

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

Successfully merging this pull request may close these issues.

4 participants