-
Notifications
You must be signed in to change notification settings - Fork 382
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(generator): merge connection options into client options #8158
Conversation
Store the Connection options so that they may be made available to merge into the Client options, and then use them to implement the four polling-policy data elements. (This is the generator version of googleapis#8090.) Rules of thumb for `Options` values: - `MergeOptions()` LHS should not include the service defaults - `MergeOptions()` RHS should include service defaults - An `OptionsSpan` should include the service defaults - stored `Options` (data members) should include the service defaults Code can then normally just look at the `CurrentOptions()`, knowing that they will include the service defaults. For example, the policy options will be available. A special exception to this, however, is when testing the connection layer in isolation (without a client), as no one has created an `OptionsSpan` in that case. We could add an `OptionsSpan` to the connection-layer calls, but it would be a no-op in practice. So, tests need to ensure required options are available. Fixes googleapis#8054.
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #8158 +/- ##
=======================================
Coverage 94.58% 94.58%
=======================================
Files 1320 1322 +2
Lines 117829 117838 +9
=======================================
+ Hits 111451 111461 +10
+ Misses 6378 6377 -1
Continue to review full report at Codecov.
|
merging in the Connection base-class options (a no-op in practice).
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Don't bother looking at the "regenerated" commits. |
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.
Thank you for separating the "generated" commits, made it much easier to review things.
Do you think it would be possible to add some sort of unit test to the generated golden files? We don't write tests for each generated library, and we explain this to ourselves by testing the golden files once. I am Okay we decide to add them in a separate PR.
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.
Thanks for doing the fix
Yes, I'll look into it. Thanks. |
Store the Connection options so that they may be made available to
merge into the Client options, and then use them to implement the
four polling-policy data elements. (This is the generator version
of #8090.)
Rules of thumb for
Options
values:MergeOptions()
LHS should not include the service defaultsMergeOptions()
RHS should include service defaultsOptionsSpan
should include the service defaultsOptions
(data members) should include the service defaultsCode can then normally just look at the
CurrentOptions()
, knowingthat they will include the service defaults. For example, the policy
options will be available. A special exception to this, however, is
when testing the connection layer in isolation (without a client), as
no one has created an
OptionsSpan
in that case. We could add anOptionsSpan
to the connection-layer calls, but it would be a no-opin practice. So, tests need to ensure required options are available.
Fixes #8054.
This change is