Conversation
backends/memory/memory_test.go
Outdated
| b := New() | ||
| tester.DoBackendTests(t, b) | ||
| tester.DoBackendTests(t, New()) | ||
| tester.DoFSWrapperTests(t, New()) |
There was a problem hiding this comment.
I would fold this into DoBackendTests, so that that one always tests everything we would want to test about a backend. It could skip certain tests if the backend does not implement an optional backend.
fs.go
Outdated
| } | ||
| n, err := bw.r.Read(p) | ||
| if err == io.EOF { | ||
| bw.r = nil |
There was a problem hiding this comment.
This should not reset. if someone calls read twice on an EOF object, it will read the whole file all over again. Only reset it when Close is close is called, after which the File object should probably be made invalid.
| return nil | ||
| } | ||
|
|
||
| // ReadDir satisfies fs.ReadDirFS |
There was a problem hiding this comment.
It might be best if we do not implement extra FS interface and instead stick to the basic fs.FS, but ReadDirFS could be a useful addition and corresponds to the List call.
I'm not sure if the single root directory restriction is problematic or not, I can imagine that we may want to store blocs with / in the future. Currently we do not clearly describe what kind of names are allowed and this could vary by backend, which is probably not a good thing.
There was a problem hiding this comment.
I'll remove the name checking. I feel like fs.ReadFileFS should stay too because it matches what Load does. What do you think?
fs.go
Outdated
| return ret, nil | ||
| } | ||
|
|
||
| // Glob satisfies fs.GlobFS |
There was a problem hiding this comment.
This basically reimplements the default fs.Glob fallback implementation. I would leave this one out.
fs.go
Outdated
|
|
||
| // AsFS casts the provided interface to a fs.FS interface if supported, | ||
| // else it wraps it to replicate its functionalities. | ||
| func AsFS(st Interface) fs.FS { |
There was a problem hiding this comment.
Since all storage methods require a context and the FS interface itself does have have a way to provide one, I suggest passing it here and holding on to it in the wrapper. Using context.Background somewhere other than main is a bad idea.
There was a problem hiding this comment.
Makes sense, I'll push a fix. Thank you!
| func (bw *fsBlobWrapper) Read(p []byte) (int, error) { | ||
| if bw.r == nil { | ||
| b, err := bw.parent.Interface.Load(bw.parent.ctx, bw.b.Name) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| bw.r = bytes.NewReader(b) | ||
| } | ||
| return bw.r.Read(p) | ||
| } | ||
| func (bw *fsBlobWrapper) Close() error { | ||
| if bw.r != nil { | ||
| bw.r = nil | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
TODO: use simpleblob.NewReader once #11 is merged
2 participants
Implement a wrapper for
InterfaceandBlob. Wrapped filesystems implementsfs.ReadFileFS,fs.ReadDirFS,fs.SubFS,fs.GlobFSandfs.StatFS.Wrapping function
func AsFS(Interface) fs.FShas been implemented as described in #2.Filesystem actions related to subdirectories will only work if the subdirectory's path is
".".