laskoviymishka commented on code in PR #2300:
URL: https://github.com/apache/iceberg-go/pull/2300#discussion_r4368746176
##########
io/io.go:
##########
@@ -217,24 +217,34 @@ type MkdirAllIO interface {
MkdirAll(path string) error
}
-// FS wraps an io/fs.FS as an IO interface.
-func FS(fsys fs.FS) IO {
+// FS wraps an io/fs.FS as an IO interface. It implements ReadFileIO only
+// if the underlying fsys supports fs.ReadFileFS. Options can configure name
+// processing without requiring a separate adapter constructor.
+func FS(fsys fs.FS, opts ...FSOption) IO {
+ adapter := ioFS{fsys: fsys}
+ for _, opt := range opts {
+ if opt != nil {
+ opt(&adapter)
+ }
+ }
+
if _, ok := fsys.(fs.ReadFileFS); ok {
- return readFileFS{ioFS{fsys, nil}}
+ return readFileFS{adapter}
}
- return ioFS{fsys, nil}
+ return adapter
}
-// FSPreProcName wraps an io/fs.FS like FS, only if fn is non-nil then
-// it is called to preprocess any filenames before they are passed to
-// the underlying fsys.
-func FSPreProcName(fsys fs.FS, fn func(string) string) IO {
- if _, ok := fsys.(fs.ReadFileFS); ok {
- return readFileFS{ioFS{fsys, fn}}
- }
+// FSOption configures an IO adapter created by FS.
+type FSOption func(*ioFS)
- return ioFS{fsys, fn}
+// WithNamePreprocessor transforms filenames before FS normalizes leading
+// slashes and passes them to the underlying fs.FS. It applies to Open, Remove,
+// and ReadFile when available. A nil preprocessor leaves names unchanged.
Review Comment:
Should the doc say `fn` must be safe for concurrent use? It runs inside
`Remove`, and the [`IO.Remove`
contract](https://github.com/apache/iceberg-go/blob/769cccb2bad4acef76f11558e65b45b10b941efe/io/io.go#L59-L63)
already requires that of implementations.
```suggestion
// and ReadFile when available. A nil preprocessor leaves names unchanged.
// fn must be safe for concurrent use by multiple goroutines.
```
##########
io/io_test.go:
##########
@@ -70,3 +70,53 @@ func TestFSPreProcNameTransformsAllOperations(t *testing.T) {
require.Equal(t, underlying.opened, underlying.read)
require.Equal(t, underlying.opened, underlying.removed)
}
+
+func TestFSDefaultAndNilOptions(t *testing.T) {
+ for _, tc := range []struct {
+ name string
+ opts []iceio.FSOption
+ }{
+ {name: "default"},
+ {name: "nil option", opts: []iceio.FSOption{nil}},
+ {name: "nil preprocessor", opts:
[]iceio.FSOption{iceio.WithNamePreprocessor(nil)}},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ underlying := &recordingFS{files: fstest.MapFS{
+ "plain.txt": &fstest.MapFile{Data:
[]byte("data")},
+ }}
+ fsys := iceio.FS(underlying, tc.opts...)
+
+ file, err := fsys.Open("/plain.txt")
Review Comment:
This call and the new ones at lines 99 and 119 need a ctx as the first
argument once #2067 is in, per its [new `Open`/`Remove`
signatures](https://github.com/apache/iceberg-go/blob/c1172a228a497aa9517f023f374fcd81cff8bf3f/io/io.go#L47-L69).
Merging this branch into #2067 (c1172a2) gives no conflict, but `go test
-count=1 -run '^TestFS' ./io/` then fails to build at all three with `not
enough arguments in call`.
##########
io/io.go:
##########
@@ -217,24 +217,34 @@ type MkdirAllIO interface {
MkdirAll(path string) error
}
-// FS wraps an io/fs.FS as an IO interface.
-func FS(fsys fs.FS) IO {
+// FS wraps an io/fs.FS as an IO interface. It implements ReadFileIO only
+// if the underlying fsys supports fs.ReadFileFS. Options can configure name
+// processing without requiring a separate adapter constructor.
Review Comment:
The "separate adapter constructor" here is the `FSPreProcName` this PR
removes, so the sentence describes the refactor rather than the API.
```suggestion
// processing; see WithNamePreprocessor.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
