Introduce abstract base class for C# service implementations #5751
Introduce abstract base class for C# service implementations #5751jtattermusch wants to merge 4 commits intogrpc:masterfrom
Conversation
|
I am not sure about the naming of the generated abstract class. Currently it's "MathBase" for service called Math. |
|
@jskeet @chrisdunelm I still need to add some tests, but I think otherwise this is ready for review. |
|
Presumably you're most interested in us reviewing the C# output? And the plan is to remove the obsolete interface before GA, so basically acting as a transition between betas? |
| } | ||
|
|
||
| // server-side abstract class | ||
| public abstract class BenchmarkServiceBase |
There was a problem hiding this comment.
I think it might be clearer to generate this without the "Base" part, given that this is the level of inheritance that most clients will actually want to deal with. So just "BenchmarkService" in this case, and "Health" above. I think the "Impl" suffix for the concrete implementation is okay though.
There was a problem hiding this comment.
I didn't use just "Health" because I think for many services the class name might collide with the namespace name (e.g. service math.Math which i think a pretty common pattern in .proto files). We didn't have this problem with the interface that has the "I" prefix.
Do you see an issue with class name being the same as the package name?
There was a problem hiding this comment.
Urgh - yes, that would be nasty. See https://blogs.msdn.microsoft.com/ericlippert/2010/03/09/do-not-name-a-class-the-same-as-its-namespace-part-one
Thinking about this further, for the server-side, this is probably okay - because we genuinely expect users to derive from it in order to implement the service. For the client side, we've already got the Client suffix, so we can presumably have an abstract HealthClient and an implementation of HealthClientImpl.
There was a problem hiding this comment.
To make this even more complicated, the name of the abstract class cannot be just Health because it is enclosed in an "umbrella" static class called "Health" (and member names cannot be same as their enclosing types).
So basically have these options:
- leave HealthBase as is
- rename the umbrella class from "Health" to "HeathGrpc". I think that would actually look good in the code (you'd do things like
class HealthImpl : HealthGrpc.Health {}andHealthGrpc.BindService(...)). The problem is that all the code using the grpc stubs would need to be updated. We might be able to afford this if we actually removed the client-side interfaces and figured out the ClientBase at the same time - because these would be breaking changes anyway. - come up with some other name of the class than "Health" or "HealthBase".
There was a problem hiding this comment.
Created #5813 to see how would option 2 look like.
There was a problem hiding this comment.
Having got my head round "this is the server side base, we're not providing an implementation" I'm in favour of it being called HealthBase, and I apologise for previous confusion.
That doesn't mean the change from Health to HealthGrpc is a bad idea - I quite like that.
|
Other than the two comments, looks fine to me. Thanks so much for putting the time into this :) |
|
On Tue, Mar 15, 2016 at 2:27 AM, Jon Skeet [email protected] wrote:
|
Well, if it's in 1.0 then it needs to be in the whole of 1.x, as removing it would be a breaking change. Anything "obsolete on launch" feels odd to me - I would definitely be happier with it being gone by 1.0. Personally I'd prefer it to just be dropped immediately - especially as the later it gets, the more people will be generating code and thus see a breakage when it is removed. |
|
Closing this as it is superseded by #5928. |
3 participants
Fixes #5371.
-- introduce an abstract base class that returns NotImplemented status code for all methods
-- mark server-side interface as obsolete
-- change existing service implementations to inherit from the new base class