Update enum naming and validation #245
Conversation
| name = name.uppercase() | ||
| var name = splitWordsToSnakeCase(preferred).let { snakeCase -> | ||
| snakeCase.substringAfter(typePrefix).takeUnless { | ||
| "^\\d|\\W".toRegex().matches(it) |
There was a problem hiding this comment.
wasn't sure about adding the \W for non-word characters. i wasn't able to test it since if I tried to add a weird character in the protobuf the project wouldn't compile, but figured I'd leave it for good measure just in case.
|
@garyp hey gary, how does this look? |
|
@garyp is this project still alive? |
There was a problem hiding this comment.
Hey @fluxxion82. Thank you so much for working on this bug fix. Sorry for letting this PR go unreviewed for so long! As you probably noticed, I hadn't had much time to devote to pbandk in the last 1.5 years. I'm now getting the project back into a maintained state.
Please let me know if you're still interested in working on this PR and plan to followup on my comments below. I completely understand if you have moved on to other things since you first submitted this PR. So if I don't hear back from you, I'll try to finish the PR myself and get it merged.
| private val kotlinKeywords = setOf( | ||
| "as", "break", "class", "continue", "do", "else", "false", "for", "fun", "if", "in", | ||
| "interface", "is", "null", "object", "package", "return", "super", "this", "throw", | ||
| "true", "try", "typealias", "typeof", "val", "var", "when", "while" | ||
| ) | ||
| private val disallowedValueTypeNames = disallowedTypeNames + kotlinKeywords + setOf( |
There was a problem hiding this comment.
❓ Why do we need to exclude kotlin keywords from the allowed value type names?
There was a problem hiding this comment.
Since I wasn't sure if this is needed, I removed it from this PR. If you have an example that requires this, let's add it as a unit test and reintroduce this change.
protoc-gen-pbandk/lib/src/commonMain/kotlin/pbandk/gen/Namer.kt
Outdated
Show resolved
Hide resolved
protoc-gen-pbandk/lib/src/commonMain/kotlin/pbandk/gen/Namer.kt
Outdated
Show resolved
Hide resolved
protoc-gen-pbandk/lib/src/commonMain/kotlin/pbandk/gen/Namer.kt
Outdated
Show resolved
Hide resolved
|
@garyp you're alive! welcome back :) I did switch things up but I'd still be interested in seeing this pr through. I'll need to find some time to get back in this and remember what I was doing but I'll try to push an update in the not too distant future. |
garyp
force-pushed
the
salbury/update_disallowed_enum_names
branch
from
d79497b to
96c447e
Compare
Enum namings could end up being named as an invalid kotlin identifier, so updated the enum name generation to avoid invalid names.
garyp
force-pushed
the
salbury/update_disallowed_enum_names
branch
from
96c447e to
77ea8c8
Compare
|
@fluxxion82 I just ran into this bug myself because the newest version of |
|
Your original PR had this change (I can't comment on it anymore since I removed it): - disallowedValueTypeNames.contains(name) ||
+ disallowedValueTypeNames.any { it.equals(name, true) } ||I found that this was being too aggressive in renaming identifiers in the existing generated code even though they weren't causing any conflicts (e.g. an enum value named |
Enum namings could end up being named as an invalid kotlin identifier, so updated the enum name generation to avoid invalid names.
Fixes #226