Proof-of-concept to allow initialisms to be set through flags. #89
Conversation
internal/plugin/plugin.go
Outdated
There was a problem hiding this comment.
Most relevant part of the change is here and the strcase.go file below.
There was a problem hiding this comment.
Hey @cory-miller, this is great! Thanks for the submission, I like where this is going.
I have a few small changes/nits that I think would ago a long ways in improving the overall UX of this new plugin option:
- Rename
InitialismsStrCase []stringtoIgnoreAcronyms []stringand theignore-strcase-initialismstoignore-acronymsor even justAcronyms/acronymsand sort alphabetically for consistency with the other Config fields - move the actual registration logic (i.e.
strcase.ConfigureAcronym()out oftoCamel/toLowerCameland into the plugin'sRunmethod - remove the
.ideadirectory and consider adding an entry to.gitignore - bonus points for updating the docs and changelog
Let me know your thoughts!
internal/plugin/strcase.go
Outdated
|
|
||
| s := fmt.Sprintf(format, args...) | ||
|
|
||
| for _, initialism := range svc.cfg.InitialismsStrCase { | ||
| if strings.Contains(s, initialism) { | ||
| strcase.ConfigureAcronym(s, s) | ||
| } | ||
| } | ||
|
|
||
| return strcase.ToCamel(s) |
There was a problem hiding this comment.
instead of calling ConfigureAcronym everytime toCamel and toLowerCamel are called, consolidate and move this logic into the plugin's Run method
There was a problem hiding this comment.
So unfortunately I don't think strcase works in the way you'd expect. Instead of letting you mask particular parts of the input, you have to mask everything. It might be easier to illustrate with an example:
https://go.dev/play/p/8UUJHeOxQSo
Maybe that should be a bug on the strcase part. I'm not sure what the intended behavior is.
There was a problem hiding this comment.
I think there's a miscommunication, I'm suggesting that we keep the same logic you've added to both toCamel and toLowerCamel, but relocate it from those methods into plugin.go's Run method.
strcase.ConfigureAcronym inserts a key/value pair into the module's uppercaseAcronyms global variable of type sync.Map, so inserting the values passed via plugin options once at plugin initialization should be sufficient, as opposed to reinserting the same values every time toCamel or toLowerCamel is called:
There was a problem hiding this comment.
The difficulty there is that I do not know what the full strcase.ConfigureAcronym should be ahead of time, unless I understand the implementation details of this tool.
Let me explain:
You have calls to strcase that look like New%sClient where we replace %s with whatever is in my protobuf. If what I want is to be able to mask what's in my protobuf (AWS can be the example for the service and ManageAWS the RPC). Then I have to put as flags:
NewAWSClientManageAWSManageAWSAsyncTestAWSClient- and many more as seen in https://github.com/search?q=repo%3Acludden%2Fprotoc-gen-go-temporal%20svc.toCamel&type=code
This would be cumbersome to find all of these. That's why I am creating new entries with strcase.ConfigureAcronym by doing partial string match in the call.
There was a problem hiding this comment.
👍 I see now, thank you for clarifying. Let me noodle on this a bit this week. It might be worth ripping out the third-party dependency and replacing it with something (first or third party) that fits this use-case better. I don't think it would be too difficult.
|
@cludden I took care of 1, 3, and 4 from #89 (review) and left a comment in #89 (comment). Another option I thought of - when calling out to the protogen library and the |
There was a problem hiding this comment.
@cory-miller huge thank you for adding this! While I noodle on replacement of iancoleman/strcase, I'm going to go ahead and this merged in 👍
This builds on top of #89 by replacing the existing usage of the `iancoleman/strcase` third-party library with a local strcase package that better supports the new ignore-acronyms plugin option and also changes the flags behavior to use semicolon delimiters for better ergonomics. In order to prevent backwards incompatible changes to generated code, the protobuf options have been expanded to support custom overrides to various cli command and field names, aliases, and usage.
This builds on top of #89 by replacing the existing usage of the `iancoleman/strcase` third-party library with a local strcase package that better supports the new ignore-acronyms plugin option and also changes the flags behavior to use semicolon delimiters for better ergonomics. In order to prevent backwards incompatible changes to generated code, the protobuf options have been expanded to support custom overrides to various cli command and field names, aliases, and usage.
Showcasing what I see in #90