Invalid generated names from enum
fluxxion82 opened this issue · 4 comments
I have a proto definition like this:
enum Variable {
NONE = 0;
VARIABLE_1 = 1;
VARIABLE_2 = 2;
VARIABLE_3 = 3;
VARIABLE_4 = 4;
}
and the resulting generated code looks like this:
public sealed class Variable(override val value: Int, override val name: String? = null) : pbandk.Message.Enum {
override fun equals(other: kotlin.Any?): Boolean = other is CheckConditionEchemDelta.Variable && other.value == value
override fun hashCode(): Int = value.hashCode()
override fun toString(): String = "CheckConditionEchemDelta.Variable.${name ?: "UNRECOGNIZED"}(value=$value)"
public object NONE : Variable(0, "NONE")
public object 1 : Variable(1, "VARIABLE_1")
public object 2 : Variable(2, "VARIABLE_2")
public object 3 : Variable(3, "VARIABLE_3")
public object 4 : Variable(4, "VARIABLE_4")
...
i know it's not great naming of the enum variable but maybe there's someway to not use numbers for the object names here.
Using pbandk version 0.14.1
That's an interesting edge case 🙂 Because of protobuf's somewhat strange scoping rules, many protobuf style guides recommend naming enum values using the name of the enum as a prefix for the enum value's name. See https://docs.buf.build/lint/rules#enum_value_prefix for an example and a more in-depth explanation. To make this common pattern less verbose when used from Kotlin, pbandk will strip off the prefix from the enum value's name when the prefix matches the enum name ("Variable" in your example above). This is done in some other popular Protobuf libraries as well.
In your case, this is resulting in names that are invalid Kotlin identifiers. We should definitely fix pbandk to avoid generating invalid identifiers. What is the preferred Kotlin name for these enum values in your situation? Would you want them to retain the "Variable" prefix and have a Kotlin name like Variable.VARIABLE_1?
I don't know that I have a preference for what the generated name is. It looks like in another project where we use grpc-java the generated names don't have the prefix stripped, but I don't think that it's necessary for anything I'm doing. So in short, no preferences.
@fluxxion82 @sonrohan I'm not sure we'll have time to work on this fix before mid-October. If either of you are interested in making a pull request for this though, I can provide guidance and advice. The code responsible for choosing the name of the enum value is here:
.My suggestion would be to keep the enum value named VARIABLE_1 in this scenario. In other words, don't strip the prefix from the name if the result does not start with a valid character for a Kotlin identifier.