-
Notifications
You must be signed in to change notification settings - Fork 375
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
feat(bigtable): add Make(Data,Admin,InstanceAdmin)Client methods that take Options #7226
feat(bigtable): add Make(Data,Admin,InstanceAdmin)Client methods that take Options #7226
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7226 +/- ##
========================================
Coverage 93.80% 93.81%
========================================
Files 1338 1338
Lines 114948 115118 +170
========================================
+ Hits 107830 107994 +164
- Misses 7118 7124 +6
Continue to review full report at Codecov.
|
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.
Some thoughts below, let me know what you think.
std::move(connection_pool_name_); | ||
} | ||
return std::move(opts_); | ||
}; |
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.
nit: semi-colon ?
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.
Fixed.
@@ -462,6 +462,16 @@ class ClientOptions { | |||
using BackgroundThreadsFactory = ::google::cloud::BackgroundThreadsFactory; | |||
BackgroundThreadsFactory background_threads_factory() const; | |||
|
|||
/// Consume this object and convert it to `Options`. | |||
Options&& ToOptions() && { |
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.
Is this something you want in the public API (and therefore maintain forever?) Consider adding a friend function in the internal namespace so it is not a public function.
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.
Yeah, there was no reason to introduce a public function here.
/** | ||
* Create a new data client configured via @p options. | ||
* | ||
* @deprecated use the `MakeDataClient` method which accepts |
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.
Do you have a issue to remind us that this should be labeled with a GOOGLE_CLOUD_CPP_DEPRECATED
macro?
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.
opened #7230
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Part of the work for #6307 (nearly there!)
This change
ClientOptions
->Options
for backwards compatibility:Make*Client(... , Options)
methods to replaceCreateDefault*Client(... , ClientOptions)
methods.internal::CommonClient
implementation to useOptions
instead ofClientOptions
Still TODO on #6307, which I will address in subsequent PR's:
Options
constructor totesting::Mock*Client
ClientOptions
fromtesting::InProcess*Client
CreateDefault*Client
in our examples/tests to useMake*Client
insteadThis change is