refactor global config into a dedicated internal package #26
Conversation
internal/config/config.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func sanitizeConfigValues(config *viper.Viper) { |
There was a problem hiding this comment.
perhaps the name of this helper function is better to be called
sanitizeSAPControlUrl` something like this
the current name of the function imply a not single-responsability and is to vague
There was a problem hiding this comment.
Yeah, I had originally done it how you're suggesting, but then I realized that we have to pass the whole configuration as a pointer argument and mutate it in place anyways, while a single responsibility function would have to be a pure one, getting a string in and putting another string out.
We can re-evaluate this in case we add more validators, but for the time being we only have one.
There was a problem hiding this comment.
I would imho suggest the contra-approach. I would for the time beeing use a not-generic function istead of generalizing it ( I guess we will never sanitize anh other urls).
I think the function name should not depend of the fact we give it a pointer as argument ( this is 2ndary).
We should rather have clearly in mind what the function does. Reading the function name, I guess it manipulate multiples stuff but in fact doesn't
There was a problem hiding this comment.
I'm speaking here only about the name
There was a problem hiding this comment.
hmm okay, fair enough! I guess YAGNI comes into place here. I'll change both names!
internal/config/config.go
Outdated
| return config, nil | ||
| } | ||
|
|
||
| func validateConfigValues(config *viper.Viper) error { |
There was a problem hiding this comment.
validateSAPConfigUrl or something like this
There was a problem hiding this comment.
same as #26 (comment)
…one value we currently deal with
Since the code about configuration was growing a bit too much in
main.go, this PR moves it into a dedicated package.It also introduces a better mechanism to set
sap-control-url: a default value is now provided (localhost:50013), and thehttp://prefix is internally added in case it's missing, to avoid unfriendly protocol errors that could previously occur.