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

Make it possible to have lowercase enums in Ruby #1965

Closed
CarlosHdez opened this issue Aug 15, 2016 · 20 comments · Fixed by #10454
Closed

Make it possible to have lowercase enums in Ruby #1965

CarlosHdez opened this issue Aug 15, 2016 · 20 comments · Fixed by #10454

Comments

@CarlosHdez
Copy link

CarlosHdez commented Aug 15, 2016

I have an enum that is something like this

enum status {
   active = 0;
   paused = 1;
}

My issue is when compiling it to a .rb I get a typeError due to this line
https://github.com/google/protobuf/blob/f53f911793c3024976f80211e0c976f5cc51f88d/ruby/ext/google/protobuf_c/message.c#L546
I get it is a standart to use uppercase for Ruby constants, but it is not used only on Ruby and changing all my constants is not a real option for me as my project is already big.
Is there a way to bypass that error.
Thanks in advance

@CarlosHdez CarlosHdez changed the title Make it possible to have lowercase enums Make it possible to have lowercase enums in Ruby Aug 15, 2016
@xfxyjwf
Copy link
Contributor

xfxyjwf commented Aug 15, 2016

Just to check if my understanding is correct: that example proto file can't be used in Ruby, but the .proto file is already used in other languages so you can't easily change the enum values to use UPPERCASE?

@CarlosHdez
Copy link
Author

I have the corresponding .rb of that .proto that, yes, is used in other languages so the change for uppercase would be a little bit difficult

@CarlosHdez
Copy link
Author

I assume this is a WontChange?

@xfxyjwf xfxyjwf added bug and removed question labels Sep 2, 2016
@xfxyjwf
Copy link
Contributor

xfxyjwf commented Sep 2, 2016

Although the proto file is not conforming to proto style guide, as long as the protocol compiler doesn't fail on such proto files, the generated ruby code should work rather than failing at runtime. This needs to be fixed in the ruby runtime or ruby code generator.

@djudd-stripe
Copy link

I'd like to request this be re-opened. Using all-lowercase symbols is a common convention in Ruby, and the natural way to map a fixed set of symbols to Proto seems to be as an enum. In my use case, it's not feasible to simply rename the enum values, because they've already been serialized in a number of places. Enforcing a style guide seems like something a linter should do, not a compiler.

If this is as simple as removing an assertion, I'm happy to submit a PR.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Jan 18, 2018

@haberman @TeBoring Can you comment on this issue? It seems to me a bug in ruby code generator.

@anandolee
Copy link
Contributor

ping @haberman @TeBoring?

@djudd-stripe
Copy link

FWIW - it's not just a question of removing an assertion, because we declare Ruby constants with the names of the enum values, and Ruby doesn't allow declaring constants with an initial lowercase letter. This still seems to me like a bug in the Ruby generator, but one that might require breaking API changes to fix. (Alternatively, maybe there's a way to just warn + avoid declaring the constants in the case where lowercase names were used.)

@TeBoring
Copy link
Contributor

Can we add a PB prefix to fix that?

@justinfeng-ap
Copy link

Hi team, I understand this issue has been hanging for a while, but wondering is there a solution to it? I can see ruby-protobuf gem introduced a env var to upcase enum name, is it an option to do something similar? cheers

@fowles
Copy link
Contributor

fowles commented Jul 1, 2022

I don't see us prioritizing this in the near term. I think in the vague future we are likely to add some features that will make this kind of configuration more acceptable, but it won't move for a while.

@tisonkun
Copy link
Contributor

+1 to add an option automatic capitalize enum name.

tisonkun added a commit to tisonkun/protobuf that referenced this issue Aug 24, 2022
@tisonkun
Copy link
Contributor

I try to write a monkey patch to handle this case #10454.

You're welcome to comment and show me how to make it solid or introduce any option.

@JasonLunn
Copy link
Contributor

Another manifestation of #10491

@Ashish-sarmah
Copy link

could someone make it clear what's the new bug in this issue. I am not very clear if this issue has been solved...

@tisonkun
Copy link
Contributor

tisonkun commented Sep 27, 2022

When working on #10454, I notice that even in current codebase we can use

YourEnumType.resolve(:lowerCaseEnums)

to get the int number. So I may prefer to close this as having a workaround.

Auto capitalize can break existing code making use of JSON codec or the method above already.

UPDATE: The original purpose here can be only to define the constant following Ruby's rule. Let me try to narrow the change scope.

UPDATE2: I think the patch #10454 is ready to merge 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

JasonLunn pushed a commit that referenced this issue Sep 28, 2022
@tisonkun
Copy link
Contributor

tisonkun commented Oct 1, 2022

@JasonLunn @mkruskal-google shall we port the change to 21.x to get it in 3.21.8, or we will release a 3.22.0 recently? Both version works for me to include the patch into my gem.

@tisonkun
Copy link
Contributor

tisonkun commented Oct 3, 2022

ping @JasonLunn @mkruskal-google

@mkruskal-google
Copy link
Member

22.x isn't scheduled yet, but it should get released by EOY. If you want to backport it to 21.x to get picked up in the next release that seems reasonable to me

@tisonkun
Copy link
Contributor

tisonkun commented Oct 4, 2022

@mkruskal-google Thank you. Pick at #10708.

mkruskal-google pushed a commit that referenced this issue Oct 13, 2022
mkruskal-google added a commit that referenced this issue Oct 19, 2022
* Force uninstall protobuf in python macos builds

We are seeing failures in brew uninstall protobuf due to no package. Change this to a force install to avoid the error.

* Fix spelling errors (#10717)

* Merge pull request #10200 from tonydnewell/bugfix/protobuf-7474

Fix for grpc.tools #17995 & protobuf #7474 (handle UTF-8 paths in argumentfile)

* Upgrade to kotlin 1.6

* 21.x No longer define no_threadlocal on OpenBSD

* Upgrade kokoro to Xcode 14 (#10732)

* Upgrade kokoro to Xcode 14

* Fix osx errors

* Merge pull request #10770 from protocolbuffers/googleberg-cl-480629524

Mark default instance as immutable first to avoid race during static initialization of default instances.

* Auto capitalize enums name in Ruby (#10454) (#10763)

This closes #1965.

* Edit toolchain to work with absl dep

* Bump upb to latest version after fixes applied (#10783)

* 21.x 202210180838 (#10785)

* Updating version.json and repo version numbers to: 21.8

* Update changelog

Co-authored-by: Protobuf Team Bot <protobuf-team-bot@google.com>

* Update generated protos

Co-authored-by: deannagarcia <69992229+deannagarcia@users.noreply.github.com>
Co-authored-by: Matt Fowles Kulukundis <matt.fowles@gmail.com>
Co-authored-by: Deanna Garcia <deannagarcia@google.com>
Co-authored-by: Brad Smith <brad@comstyle.com>
Co-authored-by: Jerry Berg <107155935+googleberg@users.noreply.github.com>
Co-authored-by: tison <wander4096@gmail.com>
Co-authored-by: Protobuf Team Bot <protobuf-team-bot@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.