Added ruby wrapper for grpc_compression_options

[apolcyn]

This adds a ruby class wraps a grpc_compression_options. The ruby wrapper has optional constructor args but is immutable.

Right now, it could be added to the ruby server_args or channel_args hashes with #merge, for example.

It's #to_hash and #to_channel_args_hash methods return compression-related key-values that can be passed to the core.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[googlebot]

CLAs look good, thanks!

[murgatroid99]

Why are the internal C functions written as Ruby functions with VALUE inputs and outputs, instead of regular C functions that work with regular C types?

[apolcyn]

My main idea with that was just so that I could test them with rspec.

I defined all of the internal C functions as private methods on the ruby
class, expect for #to_hash. They get called in the rspec tests with the
#send(:private_method_name, args..)

Was also thinking it would make it easy to make them public though too in case we wanted to.

On Fri, Jul 1, 2016 at 3:49 PM, Michael Lumish notifications@github.com
wrote:

Why are the internal C functions written as Ruby functions with VALUE
inputs and outputs, instead of regular C functions that work with regular C
types?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7216 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AJH4LijHbzvlwkPBzveS-989HSaOuvivks5qRZl2gaJpZM4JDhWF
.

[murgatroid99]

I think you should not have tests for private methods. They are, by definition, implementation details of the class, and they should not be required to be stable. The units that you test should be your public interface, and most of those functions appear to be true helper methods that wouldn't be useful to a Ruby user.

[murgatroid99]

I think it would make sense for this function to accept only symbol parameters. We are dealing with a predefined set of values that are unlikely to change in the foreseeable future, and symbols work well in that case. Plus, if you do it that way, you can predefine the symbols and compare against them directly, instead of doing string comparisons.

[murgatroid99]

You shouldn't need constants that hold symbols. The symbol itself should be a suitable constant for referring to a level. And if you get rid of the constants, you can just store the ID directly, like in the channel code (https://github.com/grpc/grpc/blob/master/src/ruby/ext/grpc/rb_channel.c#L377).

[murgatroid99]

I think you still need a return statement here.

[apolcyn]

I can add it still, but I saw this compile on OSX and linux.
I think it's building here, but certain interop tests that should be unaffected seem to be failing right now.

[murgatroid99]

Actually, I noticed that you have disabled_algorithms and is_algorithm_enabled method. I think you can use those for the tests you have without exposing any of the bitset information at all.

[apolcyn]

The latest updates remove testing of non-API methods and shrink the API too. The tests still use compression algorithm names like gzip and deflate, but should no longer test for internal enum values.

[murgatroid99]

LGTM

[apolcyn]

test this please

[apolcyn]

test this please