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
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
f195ead
fx.Annotate: make variadic params optional by default
josephinedotlee ab3ccde
Update annotated_test.go
josephinedotlee 7ea7b90
Merge branch 'master' into annotate-optional-variadics
sywhang 5a8abec
fx.Annotate: make variadic params optional by default
josephinedotlee 24c501b
Update annotated_test.go
josephinedotlee 34519fa
Temporarily pin Dig dependency to master (#827)
sywhang d173aa3
Add fx.Module (#830)
sywhang d9ebe58
fx.Module: Reorganize code (#832)
sywhang 9032ca9
Update test to add appropriate annotations
josephinedotlee 3735ca9
Finish incomplete merge
josephinedotlee 5595e7e
Only make variadic params optional if no other tags are specified
josephinedotlee 16b497e
remove print statements
josephinedotlee 1fbdc13
Update annotated.go
josephinedotlee 96c8e4c
add extra test
josephinedotlee a987c12
Fix test "Provide variadic function with no optional params"
abhinav 2926652
Explain why we're adding the optional tag
abhinav e0429a1
Merge remote-tracking branch 'origin/master' into annotate-optional-v…
abhinav f8878d1
Fix test case, verified failing on master
abhinav File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
@@ -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())), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this has no annotation applied - why is this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 variadic with a missing dependency", func(t *testing.T) { | ||
josephinedotlee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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() | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
forwhatever(...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.
There was a problem hiding this comment.
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.