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 #1121(StringSliceFlag set default value into destination) #1127
Conversation
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 now seems to be a duplicate of #1123
This definitely looks like a duplicate of #1123 @sgoroshko & @atif-konasl can you both have a conversation about whose solution you think should move forward? We can't merge both |
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.
@lynncyrin & @sgoroshko , |
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.
LGTM
ping @urfave/cli for a review here! 👋 |
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.
LGTM
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.
Thanks for pulling this fix together.
One main question here—Is this not a problem for other slice flags as well (int, int64, float)? While it's important we get fixes like this in, it's also necessary that we have consistent behavior between the flag types.
flag_string_slice.go
Outdated
@@ -116,6 +116,14 @@ func (f *StringSliceFlag) GetValue() string { | |||
|
|||
// Apply populates the flag given the flag set and environment | |||
func (f *StringSliceFlag) Apply(set *flag.FlagSet) error { | |||
|
|||
if f.Destination != nil { |
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.
Nit: These nested if
statements can be collapsed
if f.Destination != nil && f.Value != nil {
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.
Bump.
Float64SliceFlag - have no destination field |
Weird that they don't have a destination field, but definitely not a requirement for this change to fix that. |
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.
LGTM, but let’s address https://github.com/urfave/cli/pull/1127/files#discussion_r425503716
fix nested conditions |
🚢 |
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
default value does not set in Destination field in StringSliceFlag.
Which issue(s) this PR fixes:
#1121
Release Notes
(REQUIRED)