Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should we reduce the amount of generated code? #628

Closed
MrMage opened this issue Nov 13, 2019 · 4 comments
Closed

Should we reduce the amount of generated code? #628

MrMage opened this issue Nov 13, 2019 · 4 comments

Comments

@MrMage
Copy link
Collaborator

MrMage commented Nov 13, 2019

Currently, we have the following in our generated code:

public final class Echo_EchoServiceClient: GRPCServiceClient, Echo_EchoService {
  public let connection: ClientConnection
  public var serviceName: String { return "echo.Echo" }
  public var defaultCallOptions: CallOptions

  /// Creates a client for the echo.Echo service.
  ///
  /// - Parameters:
  ///   - connection: `ClientConnection` to the service host.
  ///   - defaultCallOptions: Options to use for each service call if the user doesn't provide them.
  public init(connection: ClientConnection, defaultCallOptions: CallOptions = CallOptions()) {
    self.connection = connection
    self.defaultCallOptions = defaultCallOptions
  }

Should we inline these properties (except for serviceName) into GRPCServiceClient?

@glbrntt, what do you think?

@MrMage MrMage added the nio label Nov 13, 2019
@glbrntt
Copy link
Collaborator

glbrntt commented Nov 13, 2019

GRPCServiceClient is a protocol; we're just providing conformance here. (Or do you mean turn GRPCServiceClient into a class which we inherit from?)

We could actually do away with service name and just inline it into the generated code. We lose the ability to customise the path but to be honest I don't think we actually need that. We'd also be able to get rid of a protocol as well, since GRPCServiceClient extends GRPCClient to provide the serviceName (this was required so we could have AnyServiceClient).

WDYT?

@MrMage
Copy link
Collaborator Author

MrMage commented Nov 13, 2019

Sorry, I missed that GRPCServiceClient. I guess creating a class for that would be overkill.

Not feeling strongly about the service name, either; do you think getting rid of GRPCServiceClient through that would provide a substantial benefit? Or should we keep the service name in case a user wants to know the name of the service they are talking to?

@glbrntt
Copy link
Collaborator

glbrntt commented Nov 13, 2019

Not substantial, but it tidies things up a little (and removes something from the API). Do you think users need programmatic access to the service name? If not we could just add it to the generated documentation for the client.

I don't think there's a big difference in inlining vs. not inlining the service name so I'm fine with either!

@MrMage
Copy link
Collaborator Author

MrMage commented Nov 13, 2019

🤷‍♂ I guess we could remove the service name for now — further reduces our API surface for 1.0. Can still add back that later on if needed...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants