Add support for func-types to fx.As()
#1249
Conversation
|
|
There was a problem hiding this comment.
Thanks for the PR! I have a couple comments on the implementation.
At a high level, I can't think of any particular reason why this shouldn't be allowed, but can you maybe provide some insight into why this is desired over, say, just providing a struct with a call-able method defined on it and using fx.As(new(someInterface))? Is it just cleaner in some scenarios?
| if !((at.types[i].typ.Kind() == reflect.Interface && t.Implements(at.types[i].typ)) || | ||
| t.ConvertibleTo(at.types[i].typ)) { | ||
| return nil, | ||
| nil, | ||
| fmt.Errorf("invalid fx.As: %v does not implement or is not convertible to %v", t, at.types[i]) |
There was a problem hiding this comment.
Should we make these two separate checks? This will keep the code cleaner and give better error messages.
if at.types[i].typ.Kind() == reflect.Interface {
if !t.Implements(at.types[i].typ) {
return nil, nil, fmt.Errorf("invalid fx.As: %v does not implement %v", t, at.types[i])
}
} else if !t.ConvertibleTo(at.types[i].typ) {
return nil, nil, fmt.Errorf("invalid fx.As: %v cannot be converted to %v", t, at.types[i])
}| if newOutResult.Field(i).Kind() == reflect.Func { | ||
| newOutResult.Field(i).Set(getResult(i, results).Convert(newOutResult.Field(i).Type())) | ||
| } else { | ||
| newOutResult.Field(i).Set(getResult(i, results)) | ||
| } |
There was a problem hiding this comment.
nit: should we flip these conditions to make the control flow more/appear consistent with the check above?
if newOutResult.Field(i).Kind() == reflect.Interface {
newOutResult.Field(i).Set(getResult(i, results))
} else {
newOutResult.Field(i).Set(getResult(i, results).Convert(newOutResult.Field(i).Type()))
}| @@ -1300,7 +1317,11 @@ func (at *asAnnotation) results(ann *annotated) ( | |||
|
|
|||
| newOutResult := reflect.New(resType).Elem() | |||
| for i := 1; i < resType.NumField(); i++ { | |||
There was a problem hiding this comment.
Is there any reason we would need to check Value.CanConvert first since technically Value.Convert can panic even if Type.ConvertibleTo returns true? I can't think of any reason why a func value wouldn't be convertible if its type is though.
| @@ -477,6 +480,32 @@ func TestAnnotatedAs(t *testing.T) { | |||
| assert.Equal(t, s.String(), "another stringer") | |||
| }, | |||
| }, | |||
| { | |||
| desc: "value type convertible to target type", | |||
There was a problem hiding this comment.
nit: consider downscoping the names of these cases since convertible types are locked down to just functions right now
| desc: "value type convertible to target type", | |
| desc: "function value convertible to target type", |
| }, | ||
| }, | ||
| { | ||
| desc: "anonymous value type convertible to target type", |
There was a problem hiding this comment.
should we add a test case for a function vaoue that is not convertible to the target type?
| @@ -1145,6 +1146,19 @@ var _ Annotation = (*asAnnotation)(nil) | |||
| // constructor does NOT provide both bytes.Buffer and io.Writer type; it just | |||
| // provides io.Writer type. | |||
| // | |||
| // Example for function-types: | |||
There was a problem hiding this comment.
Let's update line 1129 as well:
- // constructor) to be provided as another interface.
+ // constructor) to be provided as another type.
No description provided.