Update construct_settings overrides #103
Conversation
Based on the discussion of googleapis#95 and googleapis/gapic-generator#70 -- this changes the interface of construct_settings a bit. The new interface does not have the override parameters for retrying and bundling anymore. Instead, it has a single config_override that has the same strucure as client_config has. API users can specify a copy of default config data with a few modifications, but if the user-supplied config does not specify some data, this will fall back to the default config, so users can safely supply a part of the client config, just for their edits.
|
LGTM
|
This needs to split override_config into a few functions due to code complexity warning by PEP8.
|
Fixed the errors -- and also filed issues on php and nodejs (gax-ruby already has an issue googleapis/gax-ruby#5). |
|
LGTM |
google/gax/api_callable.py
Outdated
| if not overrides: | ||
| return config_base | ||
| new_config = copy.deepcopy(config_base) | ||
| if 'retry_codes' in overrides: |
There was a problem hiding this comment.
Just a thought -- is this really the behavior we want if a user redefines a retry_code or retry_params? For example, in Pubsub, there is a "messages" retry_params. Isn't it confusing if I define a new "messages" retry parameter set without realizing that there's already one with that name, and then accidentally override a bunch of method settings?
What if we raise an exception in this case and say that the name is reserved? Or even better, ensure that a user-provided parameter name is handled distinctly from a default parameter of the same name?
There was a problem hiding this comment.
I believe this is what we want.
Here, we want to allow users to provide a copy (with edits) of JSON config. Users can copy the JSON file, modify it as they want, and then pass it as a parameter. This is why the new override has the exactly same structure as the original config JSON.
That means, anything specified by the users has to supersede the default config. Otherwise, some of edits in the config will be silently ignored and the default values are used -- that's a more surprising behavior.
There was a problem hiding this comment.
Chatted offline and I agreed @geigerj -- the modification for the retry params will affect the overrides, but not the original ones.
Based on the discussion, now overriding config can override the specified methods only, and does not merge the configurations between the default ones and the user-provided ones.
Current coverage is 97.17%@@ master #103 diff @@
==========================================
Files 8 8
Lines 573 600 +27
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 556 583 +27
Misses 17 17
Partials 0 0
|
|
Uploaded a new patch. This (I think) represents our agreements. PTAL -- if it's the right way, I'll update other language PRs as well. |
google/gax/api_callable.py
Outdated
| bundle_descriptors, page_descriptors) | ||
| for method in overriding_settings: | ||
| if method in defaults: | ||
| defaults[method] = overriding_settings[method] |
There was a problem hiding this comment.
What about, e.g., overriding timeout but not bundling?
There was a problem hiding this comment.
fixed to support that case.
google/gax/api_callable.py
Outdated
|
|
||
|
|
||
| def _merge_retry_options(retry, overrides): | ||
| """Helper fo ``construct_settings()``. |
|
Looks good overall, just some nits |
|
LGTM |
Based on the discussion of #95 and googleapis/gapic-generator#70 --
this changes the interface of construct_settings a bit.
The new interface does not have the override parameters for
retrying and bundling anymore. Instead, it has a single
config_override that has the same strucure as client_config has.
API users can specify a copy of default config data with a few
modifications, but if the user-supplied config does not specify
some data, this will fall back to the default config, so
users can safely supply a part of the client config, just for
their edits.