Make constructors for Async*Call objects public#12877
Make constructors for Async*Call objects public#12877mehrdada merged 1 commit intogrpc:masterfrom mehrdada:publicize-call-constructors
Async*Call objects public#12877Conversation
There was a problem hiding this comment.
All 4 call types share these 4 arguments:
Task<Metadata> responseHeadersAsync, Func<Status> getStatusFunc, Func<Metadata> getTrailersFunc, Action disposeAction.
Wouldn't it be useful to wrap them in a single class (I am actually undecided what's better here)?
There was a problem hiding this comment.
The main advantage of having a class is future proofing the signature, so if we can add stuff in it in the future (which I think is a worthy goal), but if we are confident about the signature, I don't think wrapping in a class is more user-friendly here. What do you think?
There was a problem hiding this comment.
alternatively, we can provide factory methods:
currently, we have 2 factories: https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Core/Calls.cs (for "actual" calls)
and https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Core.Testing/TestCalls.cs (for test doubles).
Perhaps we could have a 3rd factory class for "intercepted" calls. That way, we would have an intermediate layer in case we wanted to make changes to Async**Call constructors.
There was a problem hiding this comment.
The ChannelExtensions in the interceptor PR acts as the third factory, but I picked the extension route to make the API nicer, i.e. enable channel.Intercept(...).
There was a problem hiding this comment.
LGTM, I think the current approach is good enough.
1.) right now I don't know of any changes that would require adding more members to Async*Call classes.
2.) we can always add an extra constructor if needed (it would only be slightly ugly)
3.) Async*Call classes have not logic in them, they only empty shells, and it should be fine to expose constructors for "data only" classes.
|
Cool. I agree. |
No description provided.