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

Add option auto_strip_enum_prefixes and enum value option scoped_alias as discussed in #1079 #1330

Closed

Conversation

warrenfalk
Copy link
Contributor

This adds file level option auto_strip_enum_prefixes and enum value option scoped_alias as discussed in feature request #1079.

This implements these options in C# and Java only, more can be implemented later, and I plan to implement at least Javascript in the future if no one else does first.

@haberman
Copy link
Member

I'm really sorry in advance that what I'm about to say will probably be disappointing, given how much thought you've given to this, and the time you put into implementing it.

I also agree that the scoping of enums is less than ideal. If we could do it all over again, we'd probably design this differently.

But I sadly have to concur with @xfxyjwf that these options are most likely something we can't merge into our repository. It's true that these are more feasible than the previous option scoped was. But on the whole, I don't think the benefit of these options is nearly worth the added complexity they would introduce.

We already have too many options. Every option represents a new path in the code generator across N different languages. This option changes the exposed API, which introduces the possibility of API incompatibilities, especially if every language doesn't implement support for it right away.

And the problem this is solving is purely cosmetic. I agree that some code would be nicer to look at if enum scoping worked better, but compared with the functional/performance issues that other options address, I'm afraid this one really doesn't pull its weight, in terms of being valuable enough for the added complexity.

@warrenfalk
Copy link
Contributor Author

OK, well allow me to plea for a reconsider, then.

First, let's drop the second option. Keeping only auto_strip_enum_prefixes will make the previously unresolvable flaw resolvable. Here's my case for that option.

How badly is it needed?

Although "purely cosmetic" is strictly true, it isn't merely that "some code would be nicer to look at". The code we're talking about ends up being highly central and referenced throughout an entire enterprise's codebase, (sometimes with public api exposure). Those who are asking for a fix are carefully designing code that is going to be forever cemented into a code base they will be expanding and supporting for years. They are trying to avoid their own "if we could do it all over again"s in the future. And in the current state of things they don't have a workaround.

How much does it impact protobuf code base?

  1. The C++ generator already calls EnumValueName() instead of enum_value->name(). This PR brings C# and Java into consistency with C++'s and there's reason to think this will eventually be necessary anyway (because the reason it is needed in C++ is to avoid keyword collision, which are also a problem in C# and Java and most languages)
  2. One new function to strip a prefix by ignoring case and underscores.
  3. The branch to call the function or not.

1 and 3 would eventually be replicated by n languages where n is the number of languages affected by the flaw. However, those without 1 already will probably eventually need it anyway. What's left is 3 which is straightforward and makes a problem plaguing many to be finally resolvable.

That's it. While it is indeed non-zero, considering that there is no other way we can work around this, it just seems well worth the branch.

How about zero additional options?

If you look at language generators as function which translates the meaning of a .proto definition into its native language, then preserving the idioms and conventions of the native language would ideally be default behavior. E.g. when someone does this in a .proto:

enum Color {
    COLOR_BLACK = 0;
    COLOR_WHITE = 1;
    COLOR_BLUE = 2;
}

The same person would, in C# write this code:

enum Color {
    Black = 0,
    White = 1,
    Blue = 2,
}

And so if the translator is doing a faithful translation, it should do the same. I realize the problem with backward compatibility at this point, which is why I suggested it as an option in the first place. I've also asked for another option to fix the case-disparity (C# only) between the two code snippets above, but now I'm guessing that hasn't a chance. However, what I'm asking is to at least have a way to have language generators generate according to their language.

Any other suggestions?

If this isn't to be accepted, are there any suggestions about what could be accepted to resolve this issue?

@goldenbull
Copy link

I agree with @warrenfalk , protoc should translate .proto into destination language in the same way as real person will do

@haberman
Copy link
Member

Thanks a lot for the feedback. Let me bounce this off some other folks internally at Google and get some more opinions on this. I do think the variant where there is no option at all seems the most promising.

@michelgb
Copy link

Hello. It is been a year since this was submitted and I am not sure if it has come to resolution. I am having the same problem. My server is in C# so my enums are like:

enum Color { COLOR_BLACK = 0; COLOR_WHITE = 1; COLOR_BLUE = 2; }
My client is an Android app in Java and I would like to get rid of the prefix. Is there any reason Java enums cannot follow the same behavior than C#? Is there an option I could use to opt-in prefix striping?

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

1 similar comment
@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@bazel-io
Copy link

Can one of the admins verify this patch?

@warrenfalk
Copy link
Contributor Author

I believe that what this pull request was doing is already the default behavior and so it is already implemented without this pull request. The latest version of the protocol buffer C# generator that I am using is already doing this. Format your ENUMS_LIKE_THIS in the .proto file an the C# generator should format them as EnumsLikeThis automatically.

@michelgb
Copy link

Hello Warren. Indeed that is the default behavior for C# but my question was why it was not implemented in Java? I was very happy with the C# behavior but to my surprise Java doesn't follow the same pattern even though both languages are very close. So let me be more specific.

I was under the impression that the recommended proto syntax for enums used all-caps separated by _, also that you can add a prefix with the same name as the enum to disambiguate in languages that doesn't support local names.

enum Color { 
   COLOR_BLACK = 0; 
   COLOR_LIGHT_BLUE = 1; 
}

I was expecting this to generate

public enum Color
    implements com.google.protobuf.ProtocolMessageEnum {
  BLACK(0),
  LIGHT_BLUE(1)
}

But instead it generates

public enum Color
    implements com.google.protobuf.ProtocolMessageEnum {
  COLOR_BLACK(0),
  COLOR_LIGHT_BLUE(1)
}

Why the prefix COLOR was not stripped when it is clear is a disambiguation that Java doesn't need? Perhaps because originally wasn't though that way and could break existing code but in that case shouldn't exist an option to control that?

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Mar 22, 2017

@michelgb There is no plan to make this change in Java. Unlike C# which is a new language in 3.0 release, Java has been opensourced for many years and we won't introduce such API breaking changes.

@xfxyjwf xfxyjwf closed this Mar 22, 2017
@cbornet
Copy link

cbornet commented Nov 22, 2018

Sorry to comment on such an old issue. But I don't understand why this was closed : the proposed change was non-breaking as it was an option, isn't it ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants