Auto capitalize enums name in Ruby

[tisonkun]

This closes #1965.

[tisonkun]

@anandolee could you help with triggering the CI workflows? If there're some failure, I can investigate during wait for reviews.

[tisonkun]

It seems all tasks passed. @haberman could you give this patch a review?

[JasonLunn]

Thanks for submitting the patch. I've highlighted a couple of concerns, but I think they're straightforward to address.

[tisonkun]

@JasonLunn thanks for your reviews. Comments addressed.

[JasonLunn]

Thanks, @tisonkun. Is the new test passing for you locally? It is failing in CI.
The JRuby changes fail with NameError: bad constant name 860 with stack trace:

add_serialized_file at [com/google/protobuf/jruby/RubyDescriptorPool.java:125]
internal_add_file at /[workspace/ruby/lib/google/protobuf/descriptor_dsl.rb:65]
add_file at /[workspace/ruby/lib/google/protobuf/descriptor_dsl.rb:43]

This exception appears in all the tests targets, including gc_test and the conformance_test, not just the unit tests.

In the CRuby interpreters, the failure is limited to the unit tests. The new assertion (assert proto_module::TestEnum::V0 == 4 fails with NameError: uninitialized constant BasicTest::TestEnum::V0 and separately the new call to push (l.push :V0) fails with RangeError: Unknown symbol value for enum field ''..

[tisonkun]

@JasonLunn how can I run tests locally? I don't find some instructions. Also the test output is missing :(

Let me try out rake test.

[tisonkun]

OK. I found that I only changed for enummodule calls, but not for the compiler. I will spend some time to find the related code and made the change. But it would help a lot if you can tell when which logic writes:

    add_enum "a.b.proto2.TestEnum" do
      value :Default, 0
      value :A, 1
      value :B, 2
      value :C, 3
      value :v0, 4
    end

[tisonkun]

For JRuby, it may need more effort. And I may consider we finish the CRuby part first and following up for the JRuby part - I work on this part-time and my use case is CRuby only.

[tisonkun]

Addressed. The ruby unit test can be still failed because updates to the compiler seems not be used by the unit test in the same patch. I'm unsure if we should make the compiler changes in first?

Hmm...I hope the CI can build the protoc and use the new one just built :)

[tisonkun]

Well. The last two commits don't help on decode_json (auto capitalize) for JRuby as described above.

[tisonkun]

@JasonLunn @deannagarcia I think ignore these cases for JRuby is OK.

1. If you define an Enum starting with lower case, it's already invalid for Ruby. We auto capitalize for using the enum without changing the proto file while keep pb binary compatible.
2. Now, if we encode to json, v0 will be V0. And it works well for Ruby self usage but fails if interop between different languages.
3. A thoroughgoing solution can be: (1) add a mark of the "original name" for codec in text/json format. (2) Use something different from Ruby's constants to represent Enum so that lower letter is allowed.

Anyway, currently, these cases pass wrongly. Even decode_json accepts enum name in lower case, it means nothing for a real Ruby use case.

[tisonkun]

@JasonLunn @deannagarcia @mkruskal-google any further comments here?

[JasonLunn]

@tisonkun - can you help me understand two things:

1. Why is it that only JRuby needs expectation changes to the conformance tests, but CRuby doesn't?
2. Can you clarify what specific tests you mean when you wrote "these cases pass wrongly"?

[tisonkun]

@JasonLunn thanks for your follow-ups:

Why is it that only JRuby needs expectation changes to the conformance tests, but CRuby doesn't?

Please read #10454 (comment). The CRuby path is full under control and can be changed, while the JRuby path depends on Java Protobuf implementation.

Can you clarify what specific tests you mean when you wrote "these cases pass wrongly"?

Even decode_json accepts enum name in lower case, it means nothing for a real Ruby use case. I'm debugging deeper in a more expressive output. Or I'd say the tests pass but it doesn't reflect a real-world use case.

[tisonkun]

Emmm..I may make a mistake here. That we can already use resolve to work with lower letter started enums.

Let me comment on the original issue.

#1965 (comment) Waiting for reviewers' suggestion.

[tisonkun]

Wait a minute. I think the original purpose is to define the constant following Ruby's rule. Let me try to narrow the change scope.

[tisonkun]

Updated. I think this patch is tidy now. All tests passed locally:

rbenv global 3.1.2
bazel test //ruby:conformance_test
rbenv global jruby-9.3.7.0
bazel test //ruby:conformance_test_jruby --define=ruby_platform=java --action_env=PATH --action_env=GEM_PATH --action_env=GEM_HOME --verbose_failures
cd ./ruby/
bundle install
bundle exec rake test
rbenv global 3.1.2
bundle exec rake test

cc @JasonLunn @deannagarcia please take a review :)

[tisonkun]

@JasonLunn @deannagarcia @mkruskal-google All tests passed now and the changeset is minimal. Please take a look :)

[JasonLunn]

Thanks for you contribution, @tisonkun!

[tisonkun]

@JasonLunn thanks for your review! I may be a bit in a hurry but I'd like to know whether/when we can have a release for this patch. I come here because my gem wants it :)

It's not a request, though. And as long as we have a determinate estimate, days or weeks should be OK.