-
Notifications
You must be signed in to change notification settings - Fork 20
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
implement futures based grpc client API #185
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good; do we need to update the version so we don't break everyone or...we're okay with temp breakage (MAJOR versus MINOR).
Well, the call_unary API that consumers use is unchanged, do we need to make a major version upgrade? |
Technically yes because of: - virtual void handle_response([[maybe_unused]] bool ok = true) override {
- // For unary call, ok is always true, `status_` will indicate error if there are any.
- m_cb(m_reply, m_status);
- }
+ virtual void handle_response(bool ok = true) = 0;
...
- unary_callback_t< RespT > m_cb; but i'll leave it up to you to decide if possibly a couple linker issues downstream are fine temporarily. |
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #185 +/- ##
==========================================
+ Coverage 62.79% 63.13% +0.33%
==========================================
Files 69 69
Lines 4209 4215 +6
Branches 531 529 -2
==========================================
+ Hits 2643 2661 +18
+ Misses 1332 1321 -11
+ Partials 234 233 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.