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

Enum prefix not dropped for enum names with consecutive capital letters #359

Open
ryanrhee opened this issue Aug 10, 2022 · 1 comment
Open
Labels
post-protobuf-es Will be worked on post migration to protobuf-es

Comments

@ryanrhee
Copy link

ryanrhee commented Aug 10, 2022

Given the following protobuf contents:

enum TSEqualsType {
  TS_EQUALS_TYPE_UNSPECIFIED = 0;
  TS_EQUALS_TYPE_EQ = 1;
  TS_EQUALS_TYPE_DOUBLE_EQ = 2;
  TS_EQUALS_TYPE_TRIPLE_EQ = 3;
}

I would expect protobuf.ts to drop the TS_EQUALS_TYPE prefix from the values. But this does not happen. It does correctly drop the prefix, if I change the enum prefix to be wrong:

enum TSEqualsType {
  T_S_EQUALS_TYPE_UNSPECIFIED = 0;
  T_S_EQUALS_TYPE_EQ = 1;
  T_S_EQUALS_TYPE_DOUBLE_EQ = 2;
  T_S_EQUALS_TYPE_TRIPLE_EQ = 3;
}

This seems like a bug in the capital snake case -> pascal snake case conversion.

ETA:
Fixing this would indeed mean that the relationship between capital snake case and pascal case is no longer one-to-one. I.e. converting TS_EQUALS_TYPE_EQ to pascal case is now ambiguous, as the options are:

  • TsEqualsTypeEq
  • TSEqualsTypeEq
  • TsEQUALSTypeEq
  • TsEqualsTYPEEq
  • etc.

But I don't think that maintaining a backwards mapping from capital snake back to pascal is necessary. At best it may be useful for some tests, but I don't think it should break for what I suspect is a fairly common case, where acronyms are used at the beginning of a enum type name.

@timostamm
Copy link
Owner

timostamm commented Aug 17, 2022

Thanks for the report, Ryan.

To find a shared prefix, we convert the enum name to SCREAMING_SNAKE_CASE first, then test whether all value names have that prefix. But the conversion from PascalCase to SCREAMING_SNAKE_CASE is very simplistic and inserts an underscore before each uppercase letter.

I don't think we need a backwards mapping. We do need the original name of each enum value for the JSON format, but we already persist the shared prefix in the generated code.

I am a bit hesitant about changing this behavior right now, because protobuf-es actually shares that shortcoming. The plan is to migrate to the base types of protobuf-es, so it makes sense to punt on a fix for a while, so we only need to fix it once. I'm labelling this with post-protobuf-es.

@timostamm timostamm added the post-protobuf-es Will be worked on post migration to protobuf-es label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-protobuf-es Will be worked on post migration to protobuf-es
Projects
None yet
Development

No branches or pull requests

2 participants