-
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
Fix retrieval of pflag stringSlice #240
Conversation
With some extra trace log formatting changes thrown into the mix. Fixes spf13#112
Don't see any issues with this, thank you for your contribution. |
@awishformore @moorereason This PR seems to fix #200 (haven't tested) But note that I changed two hunks in my workaround, one in If it's truly done, then we should probably close #200 as well. |
I'll try to take a look soon. |
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.
fix bug of string_slice with square brackets
switch flag.ValueType() { | ||
case "int", "int8", "int16", "int32", "int64": | ||
return cast.ToInt(flag.ValueString()) | ||
case "bool": | ||
return cast.ToBool(flag.ValueString()) | ||
case "stringSlice": | ||
s := strings.TrimPrefix(flag.ValueString(), "[") |
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 wrongly trims the "[" and "]" in the flags.
Fix it by:
s := flag.ValueString()
return s[1:len(s)-1]
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.
see spf13/pflag#98
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.
@shenwei356, your solution is probably faster, but strings.TrimPrefix
only removes a single "[" character. The problem with pflag was that it used strings.Trim
which would remove multiples.
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.
you are right!
With some extra trace log formatting changes thrown into the mix.
Fixes #112