-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
stages = self.stage.from_target(target) | ||
stages_info = self.stage.collect_granular(target) |
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.
Does the target
argument description string (argparse
) need an update? π
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.
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() |
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.
Maybe checking contents in dvc.yaml
directly is better?
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.
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
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.
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.
I understand the source of confusion, we surely need to improve the documentation related with this change
tests/func/test_remove.py
Outdated
assert not (tmp_dir / ".gitignore").exists() | ||
|
||
|
||
def test_remove_file_in_subdir_as_target(tmp_dir, scm, dvc): |
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.
These two tests can be merged into one? Generating a stage output both file and path, then remove them.
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.
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
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 |
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 |
Fixes #5288
Fixes #5772
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Docs: iterative/dvc.org#2357