fix: parsing config input and convert to match the type #3306
Conversation
dosubot
bot
added
the
size:M
|
@@shenqidebaozi Please review |
|
@akoserwal Okay, no problem. Let me find some more people to watch together @go-kratos/contributor |
config/options.go
Outdated
| // Default to string if no other conversion succeeds | ||
| return input | ||
| } | ||
| func expand(s string, mapping func(string) string) interface{} { |
There was a problem hiding this comment.
This is a new feature that modifies the default behavior, so it should be an optional feature.
There was a problem hiding this comment.
@shenqidebaozi How do you suggest to make it an optional feature?
There was a problem hiding this comment.
Yes, because before the feature is added, all configurations will be parsed into strings, and users may have code to handle the conversion of strings to other types. If we modify the default behavior, it may cause exceptions for users.
There was a problem hiding this comment.
I agree with your point to make it an optional behaviour.
Making it optional
- Adding an option to enable this feature: https://github.com/go-kratos/kratos/blob/main/config/options.go#L23
Like:
Changing
type Resolver func(map[string]interface{}) error
to
type Resolver func(map[string]interface{}, bool) error
and
func defaultResolver(input map[string]interface{}, enableTypeConversion bool) error
There was a problem hiding this comment.
We can leave the Resolver method definition unchanged and only make modifications within the internal implementation
There was a problem hiding this comment.
func newDefaultResolver(o bool) func(map[string]interface{}) error {
return func(input map[string]interface{}) error {
// do something
}
}
// New a config with options.
func New(opts ...Option) Config {
o := options{
decoder: defaultDecoder,
resolver: newDefaultResolver(false),
merge: func(dst, src interface{}) error {
return mergo.Map(dst, src, mergo.WithOverride)
},
}
for _, opt := range opts {
opt(&o)
}
return &config{
opts: o,
reader: newReader(o),
}
}
There was a problem hiding this comment.
You can refer to the code above
There was a problem hiding this comment.
NewDefaultResolver needs to be exported only then user will be able to make it configured like
c := config.New(
config.WithResolver(config.NewDefaultResolver(true)),
config.WithSource(
file.NewSource(flagconf),
),
)
There was a problem hiding this comment.
We can export a configuration method instead of exporting the entire implementation
There was a problem hiding this comment.
Please review @shenqidebaozi and @go-kratos/contributor
akoserwal
force-pushed
the
fix-config-parsing
branch
2 times, most recently
from
5e4599d
to
48bfca4
Compare
akoserwal
force-pushed
the
fix-config-parsing
branch
from
48bfca4
to
2ad825d
Compare
dosubot
bot
added
size:L
Description (what this PR does / why we need it):
Convert to type based on the followed pattern:
fix: #3198
Which issue(s) this PR fixes (resolves / be part of):
Other special notes for the reviewers: