Define Struct::Status in Ruby #19939
Conversation
|
1.24 release blocker (not 1.23) |
| return nil if binstatus.nil? | ||
|
|
||
| # Lazily load grpc_c and protobuf_c.so for users of this method. | ||
| require_relative './grpc' |
There was a problem hiding this comment.
nit: I think we can remove the corresponding logic from the caller of this in errors.rb
| @@ -15,6 +15,7 @@ | |||
| ssl_roots_path = File.expand_path('../../../../etc/roots.pem', __FILE__) | |||
|
|
|||
| require_relative 'grpc/errors' | |||
| require_relative 'grpc/structs' | |||
There was a problem hiding this comment.
nit: requiring this is redundant?
There was a problem hiding this comment.
Possibly, but if other Structs are moved to Ruby then they should be loaded before grpc_c. I put the require here to be explicit about it.
|
@apolcyn Would you like me to squash the commits? |
| @@ -12,9 +12,6 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| require_relative './grpc' | |||
There was a problem hiding this comment.
nit: though perhaps redundant, require structs here, since this file uses that type
| @@ -57,9 +59,6 @@ def to_status | |||
| # | |||
| # @return [Google::Rpc::Status, nil] | |||
| def to_rpc_status | |||
| # Lazily require google_rpc_status_utils to scope | |||
| # loading protobuf_c.so to the users of this method. | |||
| require_relative './google_rpc_status_utils' | |||
There was a problem hiding this comment.
nit: it's outside of the scope of this PR, but while we're here, can we also remove the two lines below here: I believe they are dead/unused code.
E.g. this could be change to just GoogleRpcStatusUtils.extract_google_rpc_status(to_status)
There was a problem hiding this comment.
Yes. I've made this change and the end2end script continues to pass.
Update error end2end test for loading when calling to_status. Update error end2end test for loading when calling to_rpc_status.
blowmage
force-pushed
the
ruby-struct-status-rb
branch
from
7ef84ce
to
0f8dced
Compare
apolcyn
added
the
release notes: yes
|
Thanks! |
This change defines
Struct::Statusin Ruby instead of ingrpc_c. This corrects an issue withGRPC::BadStatusstill depending ongrpc_cthat was discussed in #19893 (comment).