google / go-cloud

The Go Cloud Development Kit (Go CDK): A library and tools for open cloud development in Go.
https://gocloud.dev/
Apache License 2.0
9.57k stars 812 forks source link

blob: Replace `Bucket.SetIOFSCallback` #3474

Closed myaaaaaaaaa closed 3 months ago

myaaaaaaaaa commented 3 months ago

Is your feature request related to a problem? Please describe.

blob.Bucket implements io/fs.FS, but currently contains a minor papercut where SetIOFSCallback() must be called before it can be used as such.

bucket := memblob.OpenBucket(nil)
defer bucket.Close()
bucket.WriteAll(context.Background(), "foo", []byte("bar"), nil)

// oops, forgot to call SetIOFSCallback()
fmt.Println(fs.Glob(bucket, "*"))

Describe the solution you'd like

One way to prevent this would be to remove Open() from blob.Bucket so that it no longer implements io/fs.FS, and replace SetIOFSCallback() with a function that returns a io/fs.SubFS:

-func (*Bucket) SetIOFSCallback(...)
+func (*Bucket) FS(...) fs.SubFS

This would make it impossible to use a blob.Bucket as an io/fs.FS without configuring a context first.

Usage would look something like the following:

 bucket := memblob.OpenBucket(nil)
 defer bucket.Close()
 bucket.WriteAll(context.Background(), "foo", []byte("bar"), nil)

-bucket.SetIOFSCallback(func() (context.Context, *blob.ReaderOptions) {
+fs := bucket.FS(func() (context.Context, *blob.ReaderOptions) {
    return context.Background(), nil
 })

-fmt.Println(fs.Glob(bucket, "*"))    // Now a compile error
+fmt.Println(fs.Glob(fs, "*"))

This would be a breaking change.

vangent commented 3 months ago

Agreed that that would have been a better design, but I don't really want to make a breaking change at this point.

However, I think it should be easy to make it so that SetIOFSCallback isn't required, which I think is actually even better. If it's never called, we can just use context.Background() and a default ReaderOptions.