Skip to content

Conversation

ialimz
Copy link
Contributor

@ialimz ialimz commented Jul 5, 2021

This PR fixes a bug that causes the Generator not to consider Swift reserved names such as the do keyword.

For example, assume we have an RPC like below:

service JsonRpc {
    rpc Do(Request) returns (Response) {}
}

If we don't consider Swift's do keyword, we will be going to have a compile error.

With these changes, the generated code should be like below:

public protocol JsonRpc_JsonRpcClientProtocol: GRPCClient {
  var serviceName: String { get }
  var interceptors: JsonRpc_JsonRpcClientInterceptorFactoryProtocol? { get }

  func `do`(
    _ request: JsonRpc_Request,
    callOptions: CallOptions?
  ) -> UnaryCall<JsonRpc_Request, JsonRpc_Response>
}

The changes idea is the same as SwiftProtobuf library

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 5, 2021

CLA Signed

The committers are authorized under a signed CLA.

@glbrntt glbrntt self-requested a review July 5, 2021 15:32
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jul 5, 2021
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good although I've left a couple of nits.

@ialimz
Copy link
Contributor Author

ialimz commented Jul 6, 2021

Thanks @glbrntt
I've committed your suggestions

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 7, 2021

@ialimz could you run ./scripts/format.sh to make the formatting checks happy?

@glbrntt
Copy link
Collaborator

glbrntt commented Jul 7, 2021

The other failures here are #1215 -- we can ignore those for now.

@ialimz
Copy link
Contributor Author

ialimz commented Jul 8, 2021

@glbrntt SwiftFormat changes have been applied

Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Thanks @ialimz!

@glbrntt glbrntt merged commit b918cb3 into grpc:main Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants