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.

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

Auto capitalize enums name in Ruby #10454

Merged
merged 21 commits into from
Sep 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ruby/compatibility_tests/v3.0.0/tests/basic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -667,8 +667,8 @@ def test_map_msg_enum_valuetypes
assert m["z"] == :C
m["z"] = 2
assert m["z"] == :B
m["z"] = 4
assert m["z"] == 4
m["z"] = 5
assert m["z"] == 5
Comment on lines -670 to +671
Copy link
Contributor Author

Choose a reason for hiding this comment

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

4 conflicts with V0.

assert_raise RangeError do
m["z"] = :Z
end
Expand Down
9 changes: 7 additions & 2 deletions ruby/ext/google/protobuf_c/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -1263,15 +1263,20 @@ VALUE build_module_from_enumdesc(VALUE _enumdesc) {
int n = upb_EnumDef_ValueCount(e);
for (int i = 0; i < n; i++) {
const upb_EnumValueDef* ev = upb_EnumDef_Value(e, i);
const char* name = upb_EnumValueDef_Name(ev);
char* name = strdup(upb_EnumValueDef_Name(ev));
tisonkun marked this conversation as resolved.
Show resolved Hide resolved
int32_t value = upb_EnumValueDef_Number(ev);
if (name[0] < 'A' || name[0] > 'Z') {
rb_warn(
if (name[0] >= 'a' && name[0] <= 'z') {
name[0] -= 32; // auto capitalize
tisonkun marked this conversation as resolved.
Show resolved Hide resolved
} else {
rb_warn(
"Enum value '%s' does not start with an uppercase letter "
"as is required for Ruby constants.",
name);
}
}
rb_define_const(mod, name, INT2NUM(value));
free(name);
}

rb_define_singleton_method(mod, "lookup", enum_lookup, 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,10 @@ private RubyModule buildModuleFromDescriptor(ThreadContext context) {
boolean defaultValueRequiredButNotFound =
descriptor.getFile().getSyntax() == FileDescriptor.Syntax.PROTO3;
for (EnumValueDescriptor value : descriptor.getValues()) {
String name = value.getName();
// Make sure its a valid constant name before trying to create it
if (Character.isUpperCase(name.codePointAt(0))) {
String name = fixEnumConstantName(value.getName());
// Make sure it's a valid constant name before trying to create it
int ch = name.codePointAt(0);
if (Character.isUpperCase(ch)) {
enumModule.defineConstant(name, runtime.newFixnum(value.getNumber()));
} else {
runtime
Expand All @@ -189,6 +190,22 @@ private RubyModule buildModuleFromDescriptor(ThreadContext context) {
return enumModule;
}

private static String fixEnumConstantName(String name) {
if (name != null && name.length() > 0) {
int ch = name.codePointAt(0);
if (ch >= 'a' && ch <= 'z') {
// Protobuf enums can start with lowercase letters, while Ruby's constant should
// always start with uppercase letters. We tolerate this case by capitalizing
// the first character if possible.
return new StringBuilder()
.appendCodePoint(Character.toUpperCase(ch))
.append(name.substring(1))
.toString();
}
}
return name;
}

private EnumDescriptor descriptor;
private EnumDescriptorProto.Builder builder;
private IRubyObject name;
Expand Down
1 change: 1 addition & 0 deletions ruby/tests/basic_test.proto
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ enum TestEnum {
A = 1;
B = 2;
C = 3;
v0 = 4;
}

message TestEmbeddedMessageParent {
Expand Down
1 change: 1 addition & 0 deletions ruby/tests/basic_test_proto2.proto
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ enum TestEnum {
A = 1;
B = 2;
C = 3;
v0 = 4;
}

enum TestNonZeroEnum {
Expand Down
19 changes: 12 additions & 7 deletions ruby/tests/common_tests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -331,14 +331,16 @@ def test_rptfield_enum
l.push :A
l.push :B
l.push :C
assert l.count == 3
l.push :v0
assert l.count == 4
assert_raise RangeError do
l.push :D
end
assert l[0] == :A
assert l[3] == :v0

l.push 4
assert l[3] == 4
l.push 5
assert l[4] == 5
end

def test_rptfield_initialize
Expand Down Expand Up @@ -542,8 +544,8 @@ def test_map_msg_enum_valuetypes
assert m["z"] == :C
m["z"] = 2
assert m["z"] == :B
m["z"] = 4
assert m["z"] == 4
m["z"] = 5
assert m["z"] == 5
assert_raise RangeError do
m["z"] = :Z
end
Expand Down Expand Up @@ -712,14 +714,17 @@ def test_enum_lookup
assert proto_module::TestEnum::A == 1
assert proto_module::TestEnum::B == 2
assert proto_module::TestEnum::C == 3
assert proto_module::TestEnum::V0 == 4
tisonkun marked this conversation as resolved.
Show resolved Hide resolved

assert proto_module::TestEnum::lookup(1) == :A
assert proto_module::TestEnum::lookup(2) == :B
assert proto_module::TestEnum::lookup(3) == :C
assert proto_module::TestEnum::lookup(4) == :v0

assert proto_module::TestEnum::resolve(:A) == 1
assert proto_module::TestEnum::resolve(:B) == 2
assert proto_module::TestEnum::resolve(:C) == 3
assert proto_module::TestEnum::resolve(:v0) == 4
end

def test_enum_const_get_helpers
Expand Down Expand Up @@ -788,7 +793,7 @@ def test_enum_getter_only_enums
assert_raise(NoMethodError) { m.a }
assert_raise(NoMethodError) { m.a_const_const }
end

def test_repeated_push
m = proto_module::TestMessage.new

Expand Down Expand Up @@ -1762,7 +1767,7 @@ def test_freeze
assert_raise(FrozenErrorType) { m.repeated_msg = proto_module::TestMessage2.new }
assert_raise(FrozenErrorType) { m.repeated_enum = :A }
end

def test_eq
m1 = proto_module::TestMessage.new(:optional_string => 'foo', :repeated_string => ['bar1', 'bar2'])
m2 = proto_module::TestMessage.new(:optional_string => 'foo', :repeated_string => ['bar1', 'bar2'])
Expand Down
2 changes: 2 additions & 0 deletions ruby/tests/generated_code.proto
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ enum TestEnum {
A = 1;
B = 2;
C = 3;

v0 = 4;
}

message testLowercaseNested {
Expand Down
2 changes: 2 additions & 0 deletions ruby/tests/generated_code_proto2.proto
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ enum TestEnum {
A = 1;
B = 2;
C = 3;

v0 = 4;
}

message TestUnknown {
Expand Down
1 change: 1 addition & 0 deletions ruby/tests/repeated_field_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,7 @@ def fill_test_msg(test_msg)
value :A, 1
value :B, 2
value :C, 3
value :v0, 4
end
end

Expand Down