Skip to content

Make constructors for Async*Call objects public#12877

Merged
mehrdada merged 1 commit intogrpc:masterfrom
mehrdada:publicize-call-constructors
Oct 12, 2017
Merged

Make constructors for Async*Call objects public#12877
mehrdada merged 1 commit intogrpc:masterfrom
mehrdada:publicize-call-constructors

Conversation

@mehrdada
Copy link
Copy Markdown
Contributor

@mehrdada mehrdada commented Oct 6, 2017

No description provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...).

Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mehrdada
Copy link
Copy Markdown
Contributor Author

Cool. I agree.

@mehrdada
Copy link
Copy Markdown
Contributor Author

mehrdada commented Oct 12, 2017

Known issues:
Interop to cloud: #1572
Interop to prod: #12261
macOS infrastructure issue (ignored)

@mehrdada mehrdada merged commit c39b6cf into grpc:master Oct 12, 2017
@mehrdada mehrdada deleted the publicize-call-constructors branch October 12, 2017 16:18
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants