-
Notifications
You must be signed in to change notification settings - Fork 31
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: add constant-case strategy in building env key names #33
base: master
Are you sure you want to change the base?
Conversation
b.WriteRune(runes[i]) | ||
} | ||
|
||
return b.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.
With input = "helloWORLD" this algorithm will output "helloWORLD". Should it not output "hello_WORLD"?
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.
This should be helloWorld, I did this to avoid converting USA to u_s_a.
Maybe we should rename this to camelCaseToConstantCase?
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.
Why would "helloWORLD" -> "hello_WORLD" imply "USA" -> "u_s_a"?
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.
If next letter of an upper case letter is lower case, it means it has to be separated with an underline. helloWorld
-> hello_World
.
if an upper case letter is preceded by another upper case letter, it means it's something like USA
.
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.
Current behavior only checks next letters each letter, that's why we can convert something like USACountryCode
to USA_Country_Code
but we can't convert helloWORLD
to hello_WORLD
.
I'm gonna think about another algorithm.
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.
I updated the algorithm and added more test cases, please check if that's what you meant.
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.
Algorithm seems to work but is a little hard to read - what do you think about this version?
func (f *fig) toConstantCase(key string) string {
var b strings.Builder
runes := []rune(key)
lowerToUpper := func(i int) bool {
return i > 0 && unicode.IsLower(runes[i-1]) && unicode.IsUpper(runes[i])
}
upperToLower := func(i int) bool {
return i+1 < len(runes) && unicode.IsUpper(runes[i]) && unicode.IsLower(runes[i+1])
}
for i := range runes {
if i > 0 && (lowerToUpper(i) || upperToLower(i)) {
b.WriteByte('_')
}
b.WriteRune(runes[i])
}
return strings.ToUpper(b.String())
}
if f.envPrefix != "" { | ||
key = fmt.Sprintf("%s_%s", f.envPrefix, key) | ||
} | ||
return strings.ToUpper(key) | ||
} | ||
|
||
func (f *fig) toConstantCase(key 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.
This is called constant case but it's not upper casing the output.
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.
strings.Upper
is called in its parent function.
Maybe we should put the default behaviour in another strategy and be able to completely override the envKeyTransformer by providing another transformer, right?
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.
Yes
tag string | ||
timeLayout string | ||
useEnv bool | ||
useConstantCase bool |
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.
We can introduce an abstraction here so that multiple strategies can coexist without cluttering this struct and formatEnvKey
with each option:
func defaultFig() *fig {
return &fig{
envKeyTransform: strings.ToUpper
}
}
type fig struct {
envKeyTransform func(string) string
}
...
func (f *fig) formatEnvKey(key string) string {
..
key = f.envKeyTransform(key)
}
...
func UseConstantCaseForEnvKeys() Option {
return func(f *fig) {
f. envKeyTransform = toConstantCase
}
}
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.
Should we pass the env prefix to transformers? it's needed for adding prefix to the generated env key.
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.
Yeah we should add it
Closes #32
This is just a minimal implementation to see if you're ok with the way I added this new feature or not.
More test cases can be added.