fx.Annotate: make variadic params optional by default #831
Conversation
If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug.
|
|
Codecov Report
@@ Coverage Diff @@
## master #831 +/- ##
=======================================
Coverage 98.36% 98.37%
=======================================
Files 29 29
Lines 1041 1043 +2
=======================================
+ Hits 1024 1026 +2
Misses 11 11
Partials 6 6
Continue to review full report at Codecov.
|
annotated_test.go
Outdated
|
|
||
| app := fxtest.New(t, | ||
| fx.Supply( | ||
| fx.Annotate(newB(newA())), |
There was a problem hiding this comment.
Looks like this has no annotation applied - why is this Annotated?
There was a problem hiding this comment.
+1. I think fx.Annotate will no-op if there are no annotations.
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
There was a problem hiding this comment.
Let's add a test for the case where some parameters of the variadic function have annotations, but not all do.
For example, given,
func NewFoo(A, B, ...Option)
We should test at least:
fx.ParamTags(`name:"foo"`) // only A has an annotation
fx.ParamTags(`optional:"true"`, `name:"bar"`) // A and B have annotations
fx.ParamTags(`name:"foo"`, `optional:"true"`, `name:"whatever"`) // everything has an annotation
annotated.go
Outdated
| if i == ft.NumIn()-1 && ft.IsVariadic() && !strings.Contains(paramTag, "optional:") && | ||
| !strings.Contains(paramTag, "group:") { | ||
| paramTag = paramTag + " optional:\"true\"" | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ping. This comment is still valid.
annotated_test.go
Outdated
|
|
||
| app := fxtest.New(t, | ||
| fx.Supply( | ||
| fx.Annotate(newB(newA())), |
There was a problem hiding this comment.
+1. I think fx.Annotate will no-op if there are no annotations.
If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug.
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301.
This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net>
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
|
oops i broke it |
If annotate is passed a variadic function, the dependency
listed as the variadic parameter should be optional
unless otherwise specified with the
optional:falseparameter tag. This commit addresses that bug.
Resolves GO-1169