-
Notifications
You must be signed in to change notification settings - Fork 144
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
feat(AIP-123): add singular and plural validation #1143
feat(AIP-123): add singular and plural validation #1143
Conversation
Matching AIP-123 in requiring singular and plural annotations, and for singular matching the lowerCamelCase of type.
- fixing all lint problems with suggested values. - writing custom ToUpperCamelCase as existing one does not consider underscores / dashes as invalid characters, which are considered delimiters in the camel case conversion.
@noahdietz finally ready for a pass! Thanks. |
|
||
// ToUpperCamelCase returns the UpperCamelCase of a string, including removing | ||
// delimiters (_,-,., ) and using them to denote a new word. | ||
func ToUpperCamelCase(s string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooooo we could totally switch to using this but....we already use strcase
for this type of stuff: https://pkg.go.dev/github.com/stoewer/go-strcase 😬 Not sure we need this file, unless we want to drop the external/non-google dependency (which would be fine if that was a goal, I do see value in such a "clean" module approach)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strcase doesn't filter out "-" or "_" unfortunately. I did see some hacks with double-conversion to snake case, then to upperCamelCase, but I figured it might be best to roll our own that matches our own definition of "upperCamelCase".
I did find this version of strcase that matches our definition: https://github.com/iancoleman/strcase. But I decided against switching it because we need to vet that it works for the other use cases as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe what is needed is an extensible casing lib e.g. with an Option
type like WithDelimiters(delim ...string)
and ToUpperCamelCase(s string, opts ...Option) string
Anyways, thanks for the clarification. It's a tad confusing when we'd use one or the other rn, so I think we should try to replace strcase
with this sooner rather than later if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, here's a tracking issue: #1150. Agreed that mixed usage is confusing: i'll follow up to try to clean that up.
Once nice thing about our own handrolled one is we can add options as needed.
Matching AIP-123 in requiring singular and plural annotations, and for singular matching the lowerCamelCase of type.
Fixes #722, but needs guidance updated in the AIPs around the values first.