refactor: ability to run a view with a different context #1085
Conversation
| "go.opentelemetry.io/otel/trace" | ||
| ) | ||
|
|
||
| //go:generate counterfeiter -o mock/parent_context.go -fake-name ParentContext . ParentContext |
There was a problem hiding this comment.
this line could be removed after generating the fakes
There was a problem hiding this comment.
Hi @SaidAltury-ibm , it is still good to have it because one can regenerate the mocks faster when needed.
mbrandenburger
left a comment
mbrandenburger
left a comment
There was a problem hiding this comment.
Thank @adecaro! This PR contains some good cleanup and good progress for better testing.
| func WrapContext(ctx ParentContext, goContext context.Context) *WrappedContext { | ||
| return &WrappedContext{ | ||
| ParentContext: ctx, | ||
| goContext: goContext, | ||
| } | ||
| } |
There was a problem hiding this comment.
Can I suggest some better naming here and throughout the PR.
I believe we should use the variable ctx excluselivey for context.Context as this aligns pretty well with every other go code.
A view.Context, view.ParentContext, or view.ChildContext should not use ctx as variable if possible.
Example:
func WrapContext(p ParentContext, ctx context.Context) *WrappedContext {
return &WrappedContext{
ParentContext: p,
ctx: ctx,
}| @@ -25,61 +26,61 @@ type Responders struct { | |||
| Channel *session.LocalBidirectionalChannel | |||
| } | |||
|
|
|||
| type MockContext struct { | |||
| type DelegatedContext struct { | |||
There was a problem hiding this comment.
I am wondering if we can move the DelegatedContext into the integration test where it is exclusively used, and even make it private.
There was a problem hiding this comment.
I put it here because it is kind of a mock.
|
|
||
| type ctx struct { | ||
| // Context implements the view.Context interface | ||
| type Context struct { |
There was a problem hiding this comment.
I think this renaming is great!
|
@adecaro it seems that the tokenSDK does not run the tests containing this PR hyperledger-labs/fabric-token-sdk#1270 |
Signed-off-by: Angelo De Caro <[email protected]>
Signed-off-by: Angelo De Caro <[email protected]>
adecaro
force-pushed
the
f-view-manager-run-with-different-context
branch
from
f5142cb to
a7c1d7a
Compare
Signed-off-by: Angelo De Caro <[email protected]>
SaidAltury-ibm
left a comment
SaidAltury-ibm
left a comment
There was a problem hiding this comment.
Looks good. @mbrandenburger any input?
platform/view/services/view/view.go
Outdated
|
|
||
| // RunViewNow invokes the Call function of the given view. | ||
| // The view context used to the run view is either ctx or the one extracted from the options (see view.WithContext), if present. | ||
| func RunViewNow(ctx ParentContext, v View, opts ...view.RunViewOption) (res interface{}, err error) { |
There was a problem hiding this comment.
Let's quickly look at this function and apply our naming suggestions here as well to be consistent.
ctx ParentContext could become parent ParentContext.
goContext := ctx.Context could become ctx := parent.Context()
There was a problem hiding this comment.
done. I have updated also the token-sdk
Signed-off-by: Angelo De Caro <[email protected]>
mbrandenburger
changed the title
This PR introduces a new option for
Context#RunView, WithContext. WithContext allows the developer to specify acontext.Contextto use when running the view. This is very helpful when the context needs to be enhanced with additional values, for example.Tested in the token-sdk: hyperledger-labs/fabric-token-sdk#1270