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

fx.Annotate: make variadic params optional by default #831

Merged
merged 18 commits into from Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions annotated.go
Expand Up @@ -360,7 +360,7 @@ func (ann *annotated) parameters() (

// No parameter annotations. Return the original types
// and an identity function.
if len(ann.ParamTags) == 0 {
if len(ann.ParamTags) == 0 && !ft.IsVariadic() {
return types, func(args []reflect.Value) []reflect.Value {
return args
}
Expand All @@ -381,9 +381,15 @@ func (ann *annotated) parameters() (
Type: t,
}

var paramTag string
if i < len(ann.ParamTags) {
field.Tag = reflect.StructTag(ann.ParamTags[i])
paramTag = ann.ParamTags[i]
}
if i == ft.NumIn()-1 && ft.IsVariadic() && !strings.Contains(paramTag, "optional:") &&
!strings.Contains(paramTag, "group:") {
paramTag = paramTag + " optional:\"true\""
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the string matching here will be fragile. Normally, since the format of struct tags is structured, we would talk about parsing it and checking, but that might be moot because:

Based on the issue description, we want to add optional:"true" only if the variadic parameter has no other tags. So the []Foo for whatever(...Foo) defaults to optional only if the user didn't explicitly specify anything for it.
So the check we actually need is: param tags for this argument is empty, or is too short to hit this argument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping. This comment is still valid.

field.Tag = reflect.StructTag(paramTag)

inFields = append(inFields, field)
}
Expand Down
48 changes: 48 additions & 0 deletions annotated_test.go
Expand Up @@ -522,6 +522,10 @@ func TestAnnotate(t *testing.T) {
newSliceA := func(sa ...*a) *sliceA {
return &sliceA{sa}
}
newSliceAWithB := func(b *b, sa ...*a) *sliceA {
total := append(sa, b.a)
return &sliceA{total}
}

t.Run("Provide with optional", func(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -599,6 +603,50 @@ func TestAnnotate(t *testing.T) {
assert.Len(t, got.sa, 2)
})

t.Run("Provide variadic function named with no given params", func(t *testing.T) {
t.Parallel()

var got *sliceA
app := fxtest.New(t,
fx.Provide(
fx.Annotate(newSliceA, fx.ParamTags(`name:"a"`)),
),
fx.Populate(&got),
)
defer app.RequireStart().RequireStop()
require.NoError(t, app.Err())

assert.Len(t, got.sa, 0)
})

t.Run("Invoke variadic function with multiple params", func(t *testing.T) {
t.Parallel()

app := fxtest.New(t,
fx.Supply(
fx.Annotate(newB(newA())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this has no annotation applied - why is this Annotated?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. I think fx.Annotate will no-op if there are no annotations.

),
fx.Invoke(fx.Annotate(newSliceAWithB)),
)

defer app.RequireStart().RequireStop()
require.NoError(t, app.Err())
})

t.Run("Invoke non-optional variadic function with a missing dependency", func(t *testing.T) {
t.Parallel()

app := NewForTest(t,
fx.Invoke(
fx.Annotate(newSliceA, fx.ParamTags(`optional:"false"`)),
),
)
err := app.Err()
require.Error(t, err)
assert.Contains(t, err.Error(), `missing dependencies`)
assert.Contains(t, err.Error(), `missing type: []*fx_test.a`)
})

t.Run("Invoke with variadic function", func(t *testing.T) {
t.Parallel()

Expand Down