Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support hashes for struct initializers #5716

Merged
merged 4 commits into from Jul 25, 2019
Merged

Support hashes for struct initializers #5716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41e9d3b
support hashes for struct initalizers
jbolinger Feb 12, 2019
1c97d30
convert hash keys to string
jbolinger Feb 13, 2019
d60d41e
update tests
jbolinger Feb 13, 2019
0d374e9
add extra asserts
jbolinger Feb 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions ruby/ext/google/protobuf_c/map.c
View file
Open in desktop
Expand Up @@ -71,6 +71,9 @@ static VALUE table_key(Map* self, VALUE key,
case UPB_TYPE_BYTES:
case UPB_TYPE_STRING:
// Strings: use string content directly.
if (TYPE(key) == T_SYMBOL) {
Copy link
Contributor

@TeBoring TeBoring Feb 24, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also changes API of normal map fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, and it didn't break any tests that I'm aware of. Can you think of a case where it would make a difference?

This would not throw type errors in some cases where it would have previously but I don't know if that should be considered additive vs a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't break existing test cases. However, it allows symbol for string map, which is not allowed previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, is there any reason to prohibit that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes map not consistent with optional, repeated fields.
Besides, how to compare symbol? identity compare? value compare? Is the semantic same as string?
Besides, is the feature quite important? Otherwise, we don't want to over-provide it, as these trivial features necessarily makes our library complicate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also breaks existing type checking. We should added tests for our type checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allows users to provide:

Google::Protobuf::Struct.new(fields: {
   a: {number_value: 2.0}
})

instead of:

Google::Protobuf::Struct.new(fields: {
   'a' => {number_value: 2.0}
})

I would argue that Converting Symbol key values to String in Struct is a beneficial affordance, as Struct will only every have String keys. Map can have any any value type as a key, so it wouldn't make as much sense.

That said, I do think it would be fine to accept Symbol values every time a String is expected. In Ruby, I would write it like this:

class Google::Protobuf::Struct
  def [](key)
    inner_struct[String(key)]
  end
end

Basically, anytime we expect a String object, pass it to the String() method to perform the necessary conversions.

key = rb_id2str(SYM2ID(key));
}
Check_Type(key, T_STRING);
key = native_slot_encode_and_freeze_string(self->key_type, key);
*out_key = RSTRING_PTR(key);
Expand Down Expand Up @@ -395,6 +398,11 @@ VALUE Map_index_set(VALUE _self, VALUE key, VALUE value) {
void* mem;
key = table_key(self, key, keybuf, &keyval, &length);

if (TYPE(value) == T_HASH) {
VALUE args[1] = { value };
value = rb_class_new_instance(1, args, self->value_type_class);
}

mem = value_memory(&v);
native_slot_set("", self->value_type, self->value_type_class, mem, value);

Expand Down
14 changes: 14 additions & 0 deletions ruby/tests/basic.rb
View file
Open in desktop
Expand Up @@ -202,6 +202,20 @@ def test_map_field
end
end

def test_map_field_with_symbol
m = MapMessage.new
assert m.map_string_int32 == {}
assert m.map_string_msg == {}

m = MapMessage.new(
:map_string_int32 => {a: 1, "b" => 2},
:map_string_msg => {a: TestMessage2.new(:foo => 1),
b: TestMessage2.new(:foo => 10)})
assert_equal 1, m.map_string_int32[:a]
assert_equal 2, m.map_string_int32[:b]
assert_equal 10, m.map_string_msg[:b].foo
end

def test_map_inspect
m = MapMessage.new(
:map_string_int32 => {"a" => 1, "b" => 2},
Expand Down
54 changes: 54 additions & 0 deletions ruby/tests/well_known_types_test.rb
View file
Open in desktop
Expand Up @@ -139,4 +139,58 @@ def test_any
assert any.is(Google::Protobuf::Timestamp)
assert_equal ts, any.unpack(Google::Protobuf::Timestamp)
end

def test_struct_init
s = Google::Protobuf::Struct.new(fields: {'a' => Google::Protobuf::Value.new({number_value: 4.4})})
assert_equal 4.4, s['a']

s = Google::Protobuf::Struct.new(fields: {'a' => {number_value: 2.2}})
jbolinger marked this conversation as resolved.
Show resolved Hide resolved
assert_equal 2.2, s['a']
jbolinger marked this conversation as resolved.
Show resolved Hide resolved

s = Google::Protobuf::Struct.new(fields: {a: {number_value: 1.1}})
assert_equal 1.1, s[:a]
end

def test_struct_nested_init
s = Google::Protobuf::Struct.new(
fields: {
'a' => {string_value: 'A'},
'b' => {struct_value: {
fields: {
'x' => {list_value: {values: [{number_value: 1.0}, {string_value: "ok"}]}},
'y' => {bool_value: true}}}
},
'c' => {struct_value: {}}
}
)
assert_equal 'A', s['a']
assert_equal 'A', s[:a]
expected_b_x = [Google::Protobuf::Value.new(number_value: 1.0), Google::Protobuf::Value.new(string_value: "ok")]
assert_equal expected_b_x, s['b']['x'].values
assert_equal expected_b_x, s[:b][:x].values
assert_equal expected_b_x, s['b'][:x].values
assert_equal expected_b_x, s[:b]['x'].values
assert_equal true, s['b']['y']
jbolinger marked this conversation as resolved.
Show resolved Hide resolved
assert_equal true, s[:b][:y]
assert_equal true, s[:b]['y']
assert_equal true, s['b'][:y]
assert_equal Google::Protobuf::Struct.new, s['c']
assert_equal Google::Protobuf::Struct.new, s[:c]

s = Google::Protobuf::Struct.new(
fields: {
a: {string_value: 'Eh'},
b: {struct_value: {
fields: {
y: {bool_value: false}}}
}
}
)
assert_equal 'Eh', s['a']
assert_equal 'Eh', s[:a]
assert_equal false, s['b']['y']
assert_equal false, s[:b][:y]
assert_equal false, s['b'][:y]
assert_equal false, s[:b]['y']
end
end