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

FileSystem interface: provide context, make FileInfo an interface #40

Open
fstanis opened this issue May 9, 2020 · 5 comments
Open

Comments

@fstanis
Copy link

fstanis commented May 9, 2020

Just a suggestion: I was looking to potentially use this project instead of golang.org/x/net/webdav and noticed the FileSystem interface is slightly different:

  1. go-webdav doesn't pass a Context to its calls.
  2. FileInfo is a struct and not an interface.
  3. Readdir returns a []FileInfo (slice of values) but Stat returns a *FileInfo (pointer).

I was wondering if you'd consider changing this in the future? Specifically, the first two are useful for when a file system is actually remote like e.g. Dropbox or Google Drive. And using pointers in the slice is just good for consistency (and also, deep copying a slice of values has a larger overhead).

@emersion
Copy link
Owner

emersion commented May 9, 2020

go-webdav doesn't pass a Context to its calls.

Yeah, context support is probably desirable.

FileInfo is a struct and not an interface.

FileInfo is just a metadata container, why would it need to be an interface?

Readdir returns a []FileInfo (slice of values) but Stat returns a *FileInfo (pointer).

Using pointers in slices is worse in this case, because each FileInfo needs to be allocated separately and the GC needs to keep track of each pointer.

@fstanis
Copy link
Author

fstanis commented May 9, 2020

FileInfo is just a metadata container, why would it need to be an interface?

For example, for Google Drive it's convenient to keep additional information, such as an ID. Using an interface means I can reuse the same internal structure, as opposed to making a copy to be compatible with go-webdav. An interface also gives a guarantee that go-webdav doesn't mutate the structs.

I agree on the GC overhead for a slice of points, but this may be premature optimization. Specifically, I'm worried that many file system implementations would keep a cache of FileInfo objects somewhere in a different structure (e.g. a tree) and would then need to construct a slice when Readdir is called, effectively copying a lot of data. With pointers, there would only be slice construction overhead, the underlying data wouldn't be copied.

@emersion
Copy link
Owner

@emersion
Copy link
Owner

The io/fs package has been merged: https://tip.golang.org/pkg/io/fs/

emersion added a commit that referenced this issue May 2, 2022
aduffeck pushed a commit to aduffeck/go-webdav that referenced this issue Nov 14, 2023
@emersion
Copy link
Owner

context has been plumbed through now.

For example, for Google Drive it's convenient to keep additional information, such as an ID. Using an interface means I can reuse the same internal structure, as opposed to making a copy to be compatible with go-webdav. An interface also gives a guarantee that go-webdav doesn't mutate the structs.

This doesn't really help, a slice of []*customFileInfo can't be cast directly to a []webdav.FileInfo when webdav.FileInfo is an interface. Some amount of copying is required either way, and I don't think filling the webdav struct is that much work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants