feat: new overridable routing headers implementation#910
feat: new overridable routing headers implementation#910viacheslav-rostovtsev wants to merge 5 commits intogoogleapis:mainfrom
Conversation
dazuma
left a comment
dazuma
left a comment
There was a problem hiding this comment.
It would also probably be prudent to split this into two PRs, one for the gapic-common change and one for the generator template change. Because the template change depends on the gapic-common change, so we should first release a new gapic-common minor version, and then update the templates including updating the gapic-common dependency version.
| # @private | ||
| # Creates a new HeaderBinding. | ||
| # | ||
| def self.create field:, header_name: nil, regex: nil |
There was a problem hiding this comment.
Is there a reason we're creating this separate factory method rather than just writing the constructor with this signature?
There was a problem hiding this comment.
Symmetry with the routing headers extractor/binding
| @bindings = bindings || [] | ||
| end | ||
|
|
||
| def with_bindings(field:, header_name: nil, regex: nil) |
There was a problem hiding this comment.
This should be okay without the parens
|
|
||
| module Gapic | ||
| module RoutingHeaders | ||
| class HeadersExtractor |
There was a problem hiding this comment.
Unless this class is @private we need documentation for the class and its public methods.
There was a problem hiding this comment.
Will be @private
|
|
||
| if header_binding.regex && !field_value.is_a?(::String) | ||
| err_msg = "Header binding configuration is incorrect: regex" \ | ||
| "is given with a non-string field #{field}.\n" \ |
There was a problem hiding this comment.
Did you mean #{field_value}?
There was a problem hiding this comment.
yes, good catch
| next unless field_value | ||
|
|
||
| if header_binding.regex && !field_value.is_a?(::String) | ||
| err_msg = "Header binding configuration is incorrect: regex" \ |
There was a problem hiding this comment.
you probably want a space after regex.
| curr_submessage = curr_submessage.send curr_field | ||
| end | ||
|
|
||
| return curr_submessage.to_s if curr_submessage |
There was a problem hiding this comment.
You can probably replace this line with simply curr_submessage&.to_s
| field_path.each do |curr_field| | ||
| return nil unless curr_submessage.respond_to? curr_field | ||
| curr_submessage = curr_submessage.send curr_field | ||
| end |
There was a problem hiding this comment.
A bit more idiomatic would be to use Enumerable#reduce for the above:
final_submessage = field_path.reduce request do |curr_submessage, curr_field|
return nil unless curr_submessage.respond_to? curr_field
curr_submessage.send curr_field
endFollowed by:
final_submessage&.to_s2 participants
No description provided.