Continuing #762: Added trailing HTTPHeaders for custom error reporting #873
Conversation
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Great start @FranzBusch -- I've left some comments inline.
FranzBusch
force-pushed
the
ff-trailing-metadata
branch
from
9406fbf to
d98668b
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
I've left some comments about testing: unfortunately we can't use XCTUnwrap here. We should also expand the tests to cover all 4 RPC types.
In addition you'll need to update the test manifest (make generate-linuxmain should do the trick).
Sources/GRPC/CallHandlers/BidirectionalStreamingCallHandler.swift
Outdated
Show resolved
Hide resolved
FranzBusch
force-pushed
the
ff-trailing-metadata
branch
from
d98668b to
899e146
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Great stuff; you need to generate and commit the test manifests to get through the CI (make generate-linuxmain) but otherwise this looks good. I left a couple of style suggestions (missing self. etc.) too.
FranzBusch
force-pushed
the
ff-trailing-metadata
branch
from
899e146 to
ccf9335
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Looks great, subject to CI passing! Thanks a lot @FranzBusch
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Sorry @FranzBusch -- the CI was unhappy with this for Swift 5.0.
FranzBusch
force-pushed
the
ff-trailing-metadata
branch
from
ccf9335 to
aaae5a7
Compare
3 participants
I continued the PR #762 from @fabianfett. I added the changes requested by @glbrntt to use a struct instead of a tuple to hold both values. Additionally, I added tests for this new functionality. Since this is my first time contributing it would be great if you @glbrntt could point out things that I am missing or I should have done differently.
Looking forward to your feedback :)