-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Properly handle string slice values #296
Conversation
Nice, I was just coming to see if any progress had been made on this yet. Hoping to see this merged soon :-) |
@@ -880,7 +881,9 @@ func (v *Viper) find(lcaseKey string) interface{} { | |||
return cast.ToBool(flag.ValueString()) | |||
case "stringSlice": | |||
s := strings.TrimPrefix(flag.ValueString(), "[") | |||
return strings.TrimSuffix(s, "]") | |||
s = strings.TrimSuffix(s, "]") | |||
res, _ := readAsCSV(s) |
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.
Thinking out loud: maybe the error should not be silently ignored but shouted loudly as panic
? Any thoughts?
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 silently swallow errors in the other cases, so ...
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.
Ok, let's be consistent ;)
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.
Can the change be merged?
@@ -880,7 +881,9 @@ func (v *Viper) find(lcaseKey string) interface{} { | |||
return cast.ToBool(flag.ValueString()) | |||
case "stringSlice": | |||
s := strings.TrimPrefix(flag.ValueString(), "[") | |||
return strings.TrimSuffix(s, "]") | |||
s = strings.TrimSuffix(s, "]") | |||
res, _ := readAsCSV(s) |
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.
Ok, let's be consistent ;)
@bep what about the merge? ;-) |
Would love to see this be merged soon. |
@bep Hey there, just wanted to ping you again to see if you'd be interested in merging this! I'm hitting this issue in a new piece of software I'm writing, and would like to use |
The previous vendored `github.com/spf13/viper` had this bug spf13/viper#296 . This commit upgrades to the latest `github.com/spf13/viper` and closes kubernetes#1797.
The previous vendored `github.com/spf13/viper` had this bug spf13/viper#296 . This commit upgrades to the latest `github.com/spf13/viper` and closes kubernetes#1797.
Added a code handling a pflag.StringSlice values. Also tested most cases.
It fixes #200.
Similar to: #244