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

localfs: fix support for pathlib/os.PathLike objects in rm #1058

Merged
merged 1 commit into from Oct 3, 2022

Conversation

skshetry
Copy link
Contributor

@skshetry skshetry commented Oct 3, 2022

This fixes support for pathlib.Path objects in rm.

from fsspec import filesystem
from pathlib import Path

fs = filesystem("file")
fs.rm(Path("file"))

@efiop
Copy link
Member

efiop commented Oct 3, 2022

@skshetry We don't use pathobjects though.

Copy link
Member

@efiop efiop left a comment

Choose a reason for hiding this comment

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

This implementation is more correct overall, even not thinking about pathlib. 👍

@efiop efiop merged commit 07303ea into fsspec:master Oct 3, 2022
@skshetry skshetry deleted the localfs-rm branch October 3, 2022 11:50
@efiop
Copy link
Member

efiop commented Oct 3, 2022

Oops, I think I confused this with dvc-objects.fs in my comments 😅 Still, this is indeed more correct.

@skshetry
Copy link
Contributor Author

skshetry commented Oct 3, 2022

LocalFileSystem does support os.PathLike objects, via LocalFileSystem._strip_protocol -> stringify_paths. This has nothing to do with DVC, just fixing support.

@efiop efiop added the bug Something isn't working label Oct 3, 2022
@martindurant
Copy link
Member

OK, this makes sense. I bet there are other instances of this in the codebase.

@skshetry
Copy link
Contributor Author

skshetry commented Oct 3, 2022

They are in AbstractFileSystem, especially in put/expand_path/pipe/cat/get. Will have to look into them separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants