Allow client to specify metadata per call #356
Conversation
There was a problem hiding this comment.
Thank you for this change!
Please remove the existing methods that do not take a metadata argument from the protocol and add them in an extension instead. That extension can then call through to the new methods, so that we don't need to have separate implementations for the metadata and non-metadata case (because the non-metadata version simply calls through the other one with self.metadata as the metadata argument).
Also, I think the tests (and possibly some auxiliary code as well and/or the test stubs) will need to be updated to make CI pass.
| @@ -144,11 +144,19 @@ internal final class Echo_EchoServiceClient: ServiceClientBase, Echo_EchoService | |||
| return try Echo_EchoGetCallBase(channel) | |||
| .run(request: request, metadata: metadata) | |||
| } | |||
| internal func get(_ request: Echo_EchoRequest, metadata customMetadata: Metadata?) throws -> Echo_EchoResponse { | |||
There was a problem hiding this comment.
If you implement my suggestion in the review comments, it should be possible to always have this method take a non-optional Metadata argument.
There was a problem hiding this comment.
Please use Metadata as the argument for all of these, not Metadata? (in Generator.swift).
|
@MrMage Let me know if I missed or did something wrong. As a swift noob, I would follow all your suggestions to learn. Thank you. :) |
| @@ -144,11 +144,19 @@ internal final class Echo_EchoServiceClient: ServiceClientBase, Echo_EchoService | |||
| return try Echo_EchoGetCallBase(channel) | |||
| .run(request: request, metadata: metadata) | |||
| } | |||
| internal func get(_ request: Echo_EchoRequest, metadata customMetadata: Metadata?) throws -> Echo_EchoResponse { | |||
There was a problem hiding this comment.
Please use Metadata as the argument for all of these, not Metadata? (in Generator.swift).
| @@ -33,20 +33,42 @@ fileprivate struct _GeneratedWithProtocGenSwiftVersion: SwiftProtobuf.ProtobufAP | |||
| typealias Version = _2 | |||
| } | |||
|
|
|||
| struct Echo_EchoRequest: SwiftProtobuf.Message { | |||
| static let protoMessageName: String = _protobuf_package + ".EchoRequest" | |||
| struct Echo_EchoRequest { | |||
There was a problem hiding this comment.
Where do the changes in this file come from?
There was a problem hiding this comment.
Possibly using a different version of SwiftProtobuf? I think this file's changes should probably be dropped
|
Sorry, I thought I had seen changes where you made Also, why did the |
|
I agree.
… On 23. Jan 2019, at 01:59, Michael Rebello ***@***.***> wrote:
@rebello95 commented on this pull request.
In Sources/Examples/Echo/Generated/echo.pb.swift:
> @@ -33,20 +33,42 @@ fileprivate struct _GeneratedWithProtocGenSwiftVersion: SwiftProtobuf.ProtobufAP
typealias Version = _2
}
-struct Echo_EchoRequest: SwiftProtobuf.Message {
- static let protoMessageName: String = _protobuf_package + ".EchoRequest"
+struct Echo_EchoRequest {
Possibly using a different version of SwiftProtobuf? I think this file's changes should probably be dropped
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Any updates on this @xissy? Would be great to get this feature in! |
|
Sorry, I've been busy, will update soon. |
|
|
||
| } | ||
|
|
||
| internal extension Echo_EchoService { |
There was a problem hiding this comment.
@MrMage I know you had suggested putting these functions in a separate extension, but it seems like this adds a bit of boilerplate. Would it make sense to update any generated callers of these functions instead of generating more functions as an extension?
There was a problem hiding this comment.
@rebello95 I think these methods are actually called by gRPC users, not by generated code. I'm not super happy about this boilerplate, either, but I don't know how to avoid it if we don't want to force our library's consumers to include this argument with each gRPC call they are going to call. I might be missing something, though; let me know.
There was a problem hiding this comment.
Looks good to me, but I'd like to finish the discussion with @rebello95 about the generated-code boilerplate (which I don't think could be avoided) this adds before merging.
To address #355 (comment). It also closes #190.