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

remove: Add support for removing tracked files / directories #5757

Closed
wants to merge 17 commits into from

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Apr 1, 2021

Fixes #5288
Fixes #5772

Docs: iterative/dvc.org#2357

dvc/repo/remove.py Outdated Show resolved Hide resolved
dvc/repo/remove.py Outdated Show resolved Hide resolved
tests/func/test_remove.py Outdated Show resolved Hide resolved
@daavoo daavoo requested a review from skshetry April 6, 2021 07:56
stages = self.stage.from_target(target)
stages_info = self.stage.collect_granular(target)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the target argument description string (argparse) need an update? πŸ™‚

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, although I think I should wait for some clarifications regarding the doc update (iterative/dvc.org#2357) before updating the argument description


dvc.remove("foo")
assert "/foo" not in get_gitignore_content()
assert "/bar" in get_gitignore_content()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe checking contents in dvc.yaml directly is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I fully understand your suggestion. As far as I know, using dvc.remove("foo") will not update the contents in dvc.yaml; it will remove foo from both the file system and the .gitignore

Copy link
Contributor

Choose a reason for hiding this comment

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

This confused me, because according to doc dvc remove delete stages or .dvc files, including removing .dvc file or removing stages in dvc.yaml and removing outputs from .gitignore. I don't know the exact behavior of this feature, and seems that it wasn't defined clearly in documents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the source of confusion, we surely need to improve the documentation related with this change

assert not (tmp_dir / ".gitignore").exists()


def test_remove_file_in_subdir_as_target(tmp_dir, scm, dvc):
Copy link
Contributor

Choose a reason for hiding this comment

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

These two tests can be merged into one? Generating a stage output both file and path, then remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt like the 2 tests are better separated because one is asserting a successful behavior and the other is asserting that a specific exception is being raised

@jorgeorpinel
Copy link
Contributor

Will this fix #2575? If so please link them in the description of this PR.

@daavoo daavoo changed the title remove: Add support for file. remove: Add support for removing tracked files / directories Apr 13, 2021
@daavoo
Copy link
Contributor Author

daavoo commented Apr 13, 2021

Will this fix #2575? If so please link them in the description of this PR.

It will not fix the original premise of the issue (you still have to use dvc gc for cleaning the cache) but it will not raise the exception described by #2575 (comment) and work as intended in that case.

@daavoo daavoo marked this pull request as draft May 31, 2021 11:51
@efiop
Copy link
Member

efiop commented Jul 18, 2021

Unfortunately, even though this works, but the code is convoluted πŸ™ We'll need a more general solution once we have a better architecture in this department. Closing this in favor of the path outlined in #6300

@efiop efiop closed this Jul 18, 2021
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: granular support remove: support file as a target
5 participants