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

cli: remove: fix error message for the case when file is not under control (#4497) #5236

Merged
merged 4 commits into from
Jan 19, 2021

Conversation

DiPaolo
Copy link
Contributor

@DiPaolo DiPaolo commented Jan 7, 2021

Partially fix #4497

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

dvc/command/remove.py Outdated Show resolved Hide resolved
dvc/command/remove.py Outdated Show resolved Hide resolved
dvc/command/remove.py Outdated Show resolved Hide resolved
@skshetry
Copy link
Member

ping @DiPaolo, is there anything that we could do to move this PR forward?

@DiPaolo
Copy link
Contributor Author

DiPaolo commented Jan 13, 2021

@skshetry I made fixes according to your notes. I've improved error message for the user in that specific case, because IMHO it makes sense to provide the user more information. The text has been taken from your reply in the issue itself. Now it looks like:
ERROR: failed to remove 'data/description.json'. Make sure the target is either a .dvc file or a name of the stage

@skshetry
Copy link
Member

skshetry commented Jan 13, 2021

@DiPaolo, we need to support file as a target in the remove, so I am not sure if this new error message makes sense.

We do need to clean up the message, especially the "failed to remove" one. So, either we need to add support for it, or just do the cleanup in this PR.

Regarding the support for files, the change should be pretty trivial, we just need collect_granular here, similar to what's in the repo.commit:

stages_info = self.stage.collect_granular(

@DiPaolo
Copy link
Contributor Author

DiPaolo commented Jan 13, 2021

@skshetry let me give it another try... I prefer the second option you've suggested - just to clean-up the message. Adding support of files as a target is out of the scope of this issue. So, to track it correctly, I will create a separate issue once the PR is accepted, if you don't mind.

dvc/stage/exceptions.py Outdated Show resolved Hide resolved
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @DiPaolo. 🙂

@skshetry skshetry merged commit 8b0e0b9 into iterative:master Jan 19, 2021
@DiPaolo
Copy link
Contributor Author

DiPaolo commented Jan 19, 2021

@skshetry let me give it another try... I prefer the second option you've suggested - just to clean-up the message. Adding support of files as a target is out of the scope of this issue. So, to track it correctly, I will create a separate issue once the PR is accepted, if you don't mind.

Here it is #5288

@DiPaolo DiPaolo deleted the 4497 branch January 20, 2021 06:31
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.

remove one of many stage outputs: misleading error message
2 participants