Skip to content

fs: local: Accept list of paths in rm. #176

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 16, 2023
Merged

fs: local: Accept list of paths in rm. #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/dvc_objects/fs/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,13 @@ def rmdir(self, path: AnyFSPath) -> None:
def rm_file(self, path: AnyFSPath) -> None:
self.fs.rm_file(path)

def rm(self, path: AnyFSPath) -> None:
self.fs.rm(path, recursive=True)
Copy link
Contributor Author

@daavoo daavoo Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Was there a reason to hardcode recursive=True here?

It doesn't comply with fsspec and was introducing even more overhead in the underlying expand_path call for each dvc-{x} filesystem

Copy link
Contributor

@pmrowla pmrowla Jan 12, 2023

Choose a reason for hiding this comment

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

I'm not actually sure, given that we also define LocalFilesystem.rm(remove=False)

I think we should be exposing the flag properly here and defaulting to false

Copy link
Contributor

@efiop efiop Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The reason is that our old filesystem's (aka remote aka tree) rm method was always recursive. Might be not needed anymore, but that will require checking

Copy link
Contributor Author

@daavoo daavoo Jan 16, 2023

Choose a reason for hiding this comment

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

I checked dvc repo and the only place potentially affected I have found is in output (treeverse/dvc#8825)

Copy link
Contributor

@efiop efiop Jan 16, 2023

Choose a reason for hiding this comment

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

@daavoo Thank you for checking it!

def rm(
self,
path: Union[AnyFSPath, List[AnyFSPath]],
recursive: bool = False,
**kwargs,
) -> None:
self.fs.rm(path, recursive=recursive, **kwargs)

remove = rm

Expand Down
5 changes: 4 additions & 1 deletion src/dvc_objects/fs/local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ def rm_file(self, path):
remove(path)

def rm(self, path, recursive=False, maxdepth=None):
remove(path)
if isinstance(path, str):
path = [path]
for p in path:
remove(p)

def cp_file(self, path1, path2, **kwargs):
return self.copy(path1, path2, **kwargs)
Expand Down
10 changes: 10 additions & 0 deletions tests/fs/test_localfs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ def test_local_fs_isfile(tmp_path):
assert not fs.isfile(fspath(tmp_path / "not-existing-file"))


def test_local_fs_rm(tmp_path):
(tmp_path / "file").write_text("file", encoding="utf8")
(tmp_path / "file2").write_text("file2", encoding="utf8")

fs = LocalFileSystem()
fs.remove([tmp_path / "file", tmp_path / "file2"])
assert not fs.exists(tmp_path / "file")
assert not fs.exists(tmp_path / "file2")


def convert_to_sets(walk_results):
return [
(root, set(dirs), set(nondirs)) for root, dirs, nondirs in walk_results
Expand Down