Add WaitServer #23
Add WaitServer #23
Conversation
chasefleming
requested review from
Kay-Zee,
janezpodhostnik,
sideninja,
nvdtf,
bluesign,
bjartek,
jribbink and
ianthpun
as code owners
gateway/grpc.go
Outdated
| @@ -206,6 +206,11 @@ func (g *GrpcGateway) Ping() error { | |||
| return g.client.Ping(ctx) | |||
| } | |||
|
|
|||
| func (g *GrpcGateway) WaitServer() error { | |||
There was a problem hiding this comment.
do we want the context to come in as an arugment?
There was a problem hiding this comment.
I was just keeping it the same pattern as Ping. Do you think it's necessary?
There was a problem hiding this comment.
Best practise is to always send in ctx as first arg when doing network calls.
I have a pr somewhere to add ctx to the other calls i remember.
There was a problem hiding this comment.
yeah I think we need to change all to have context in general. not sure about how we version here, but this is a breaking change.
There was a problem hiding this comment.
how about we merge this as is, then we'll do the bigger breaking change into 2.0?
There was a problem hiding this comment.
Yeah if we want to add context to Ping we should do it on the stable cadence branch. We can do it in WaitServer since it's new.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
- Coverage 58.76% 58.73% -0.03%
==========================================
Files 29 29
Lines 2202 2203 +1
==========================================
Hits 1294 1294
- Misses 722 723 +1
Partials 186 186
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Issue: #22