-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix: parsing config input and convert to match the type #3306
base: main
Are you sure you want to change the base?
Conversation
@@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new feature that modifies the default behavior, so it should be an optional feature.
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.
@shenqidebaozi How do you suggest to make it an optional feature?
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, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave the Resolver method definition unchanged and only make modifications within the internal implementation
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.
@go-kratos/contributor please review
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.
@akoserwal We don't want to expose the underlying implementation, we just need to add a WithResolveSpecificType
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.
please fix lint
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.
@akoserwal We don't want to expose the underlying implementation, we just need to add a
WithResolveSpecificType
or ResolveActualTypes
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.
The actual type resolver can be configured like:
c := config.New(
config.WithResolveActualTypes(true),
config.WithSource(
file.NewSource(flagconf),
),
)
48bfca4
to
2ad825d
Compare
2ad825d
to
d2baffa
Compare
d2baffa
to
0dd92a4
Compare
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: