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

Add all FSMap constructor arguments as class attributes #939

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

rabernat
Copy link
Contributor

@rabernat rabernat commented Apr 4, 2022

There are currently two constructor attributes for FSMap - create and check - that are not accessible as instance attributes. This makes it hard to copy an FSMap. Having these accessible would be useful for e.g. zarr-developers/zarr-python#911; it would allow us to completely rebuild an FSMap object from another.

@rabernat
Copy link
Contributor Author

rabernat commented Apr 4, 2022

In looking at this code, the following scares me

if check:
if not self.fs.exists(root):
raise ValueError(
"Path %s does not exist. Create "
" with the ``create=True`` keyword" % root
)
self.fs.touch(root + "/a")
self.fs.rm(root + "/a")

What if there is an array called /a in the store? Should this check path be sure to use a file that doesn't already exist?

@martindurant
Copy link
Member

Good idea, a long unprobable string might be good. Do we really need check, I wonder.

@rabernat
Copy link
Contributor Author

rabernat commented Apr 4, 2022

I have used check in the past to verify that the store is actually writeable. I believe that wrapping the touch in a try / except block and providing a very verbose error message would be even more useful.

@martindurant
Copy link
Member

Fair enough

@martindurant martindurant merged commit 6db19d1 into fsspec:master Apr 4, 2022
@martindurant
Copy link
Member

The rest we can leave for a follow-up

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.

2 participants