Skip to content
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.

Sign up for GitHub

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

Jump to bottom

String slice issue v1 #981

Merged
merged 8 commits into from
Feb 26, 2020
Merged

String slice issue v1 #981

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c34587c
fix default value persisting after flag is initialized
asahasrabuddhe Dec 7, 2019
9a9e396
add tests
asahasrabuddhe Dec 7, 2019
a5a927c
fix failing tests
asahasrabuddhe Dec 8, 2019
9b12ddb
add more scenarios
asahasrabuddhe Dec 8, 2019
dfb55ca
improve remove from slice functions
asahasrabuddhe Jan 26, 2020
94e50b3
update remove val from int slice function
asahasrabuddhe Jan 26, 2020
809a71d
handle potential panics
asahasrabuddhe Jan 26, 2020
0f5ca21
Update slice flags
asahasrabuddhe Jan 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 52 additions & 1 deletion flag_int64_slice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ func (f *Int64Slice) Set(value string) error {

asahasrabuddhe marked this conversation as resolved.
Show resolved Hide resolved
// String returns a readable representation of this value (for usage defaults)
func (f *Int64Slice) String() string {
return fmt.Sprintf("%#v", *f)
slice := make([]string, len(*f))
for i, v := range *f {
slice[i] = strconv.FormatInt(v, 10)
}

return strings.Join(slice, ",")
}

// Value returns the slice of ints set by this flag
Expand Down Expand Up @@ -110,6 +115,7 @@ func (f Int64SliceFlag) ApplyWithError(set *flag.FlagSet) error {
}
set.Var(f.Value, name, f.Usage)
})

return nil
}

Expand All @@ -135,7 +141,52 @@ func lookupInt64Slice(name string, set *flag.FlagSet) []int64 {
if err != nil {
return nil
}
// extract default value from the flag
var defaultVal []int64
for _, v := range strings.Split(f.DefValue, ",") {
iV, _ := strconv.ParseInt(v, 10, 64)
asahasrabuddhe marked this conversation as resolved.
Show resolved Hide resolved
defaultVal = append(defaultVal, iV)
}
// if the current value is not equal to the default value
// remove the default values from the flag
if !isInt64SliceEqual(parsed, defaultVal) {
for _, v := range defaultVal {
parsed = removeFromInt64Slice(parsed, v)
}
}
return parsed
}
return nil
}

func removeFromInt64Slice(slice []int64, val int64) (newVal []int64) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We shouldn't be using named returns unless the function signature is ambiguous, or we absolutely have to

There are caveats with the scope of a named variable that aren't always intuitive, and naked returns pretty much always obfuscate what code should be doing. It also encourages us to mutate an existing variable rather than returning directly.

I'd probably write the function like this:

func removeFromInt64Slice(slice []int64, val int64) ([]int64) {
	for i, v := range slice {
		if v == val {
			return slice[count+1:]
		}
	}
	return slice
}

That said, there's an issue here, which I'm not sure if is intentional or not. This function removes all items up to and including the first occurrence of val, but the name implies that we would just be removing val. I don't think it manifests because of how this function is called, but we should either fix the implementation or rename the function to avoid future misuse of this helper.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the function to behave like it's named

func removeFromInt64Slice(slice []int64, val int64) []int64 {
	for i, v := range slice {
		if v == val {
			return append(slice[:i], slice[i+1:]...)
		}
	}
	return slice
}

See it in action: https://play.golang.org/p/ZjJxp38tKNQ

var count int
for _, v := range slice {
asahasrabuddhe marked this conversation as resolved.
Show resolved Hide resolved
if v == val {
newVal = slice[count+1:]
return
}
newVal = append(newVal, v)
count++
}
return
}

func isInt64SliceEqual(newValue, defaultValue []int64) bool {
// If one is nil, the other must also be nil.
if (newValue == nil) != (defaultValue == nil) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time I every see an expression like this in golang. 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woooooooooah, love it!

return false
}

if len(newValue) != len(defaultValue) {
return false
}

for i, v := range newValue {
if v != defaultValue[i] {
return false
}
}

return true
}
52 changes: 51 additions & 1 deletion flag_int_slice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ func (f *IntSlice) Set(value string) error {

// String returns a readable representation of this value (for usage defaults)
func (f *IntSlice) String() string {
return fmt.Sprintf("%#v", *f)
slice := make([]string, len(*f))
for i, v := range *f {
slice[i] = strconv.Itoa(v)
}

return strings.Join(slice, ",")
}

// Value returns the slice of ints set by this flag
Expand Down Expand Up @@ -136,7 +141,52 @@ func lookupIntSlice(name string, set *flag.FlagSet) []int {
if err != nil {
return nil
}
// extract default value from the flag
var defaultVal []int
for _, v := range strings.Split(f.DefValue, ",") {
iV, _ := strconv.Atoi(v)
asahasrabuddhe marked this conversation as resolved.
Show resolved Hide resolved
defaultVal = append(defaultVal, iV)
}
// if the current value is not equal to the default value
// remove the default values from the flag
if !isIntSliceEqual(parsed, defaultVal) {
for _, v := range defaultVal {
parsed = removeFromIntSlice(parsed, v)
}
}
return parsed
}
return nil
}

func removeFromIntSlice(slice []int, val int) (newVal []int) {
asahasrabuddhe marked this conversation as resolved.
Show resolved Hide resolved
var count int
for _, v := range slice {
if v == val {
newVal = slice[count+1:]
return
}
newVal = append(newVal, v)
count++
}
return
}

func isIntSliceEqual(newValue, defaultValue []int) bool {
// If one is nil, the other must also be nil.
if (newValue == nil) != (defaultValue == nil) {
return false
}

if len(newValue) != len(defaultValue) {
return false
}

for i, v := range newValue {
if v != defaultValue[i] {
return false
}
}

return true
}
62 changes: 53 additions & 9 deletions flag_string_slice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func (f *StringSlice) Set(value string) error {

// String returns a readable representation of this value (for usage defaults)
func (f *StringSlice) String() string {
return fmt.Sprintf("%s", *f)
return strings.Join(*f, ",")
}

// Value returns the slice of strings set by this flag
Expand All @@ -32,14 +32,14 @@ func (f *StringSlice) Get() interface{} {

// StringSliceFlag is a flag with type *StringSlice
type StringSliceFlag struct {
Name string
Usage string
EnvVar string
FilePath string
Required bool
Hidden bool
TakesFile bool
Value *StringSlice
Name string
Usage string
EnvVar string
FilePath string
Required bool
Hidden bool
TakesFile bool
Value *StringSlice
}

// String returns a readable representation of this value
Expand Down Expand Up @@ -132,7 +132,51 @@ func lookupStringSlice(name string, set *flag.FlagSet) []string {
if err != nil {
return nil
}
// extract default value from the flag
var defaultVal []string
for _, v := range strings.Split(f.DefValue, ",") {
defaultVal = append(defaultVal, v)
}
// if the current value is not equal to the default value
// remove the default values from the flag
if !isStringSliceEqual(parsed, defaultVal) {
for _, v := range defaultVal {
parsed = removeFromStringSlice(parsed, v)
}
}
return parsed
}
return nil
}

func removeFromStringSlice(slice []string, val string) (newVal []string) {
var count int
for _, v := range slice {
if v == val {
newVal = slice[count+1:]
return
}
newVal = append(newVal, v)
count++
}
return
}

func isStringSliceEqual(newValue, defaultValue []string) bool {
// If one is nil, the other must also be nil.
if (newValue == nil) != (defaultValue == nil) {
return false
}

if len(newValue) != len(defaultValue) {
return false
}

for i, v := range newValue {
if v != defaultValue[i] {
return false
}
}

return true
}
asahasrabuddhe marked this conversation as resolved.
Show resolved Hide resolved
93 changes: 93 additions & 0 deletions flag_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,3 +1330,96 @@ func TestFlagFromFile(t *testing.T) {
}
}
}

func TestSliceFlag_WithDefaults(t *testing.T) {
asahasrabuddhe marked this conversation as resolved.
Show resolved Hide resolved
tests := []struct {
args []string
app *App
}{
{
args: []string{""},
app: &App{
Flags: []Flag{
StringSliceFlag{Name: "names, n", Value: &StringSlice{"john"}},
IntSliceFlag{Name: "userIds, u", Value: &IntSlice{3}},
Int64SliceFlag{Name: "phoneNumbers, p", Value: &Int64Slice{123456789}},
},
Action: func(ctx *Context) error {
expect(t, len(ctx.StringSlice("n")), 1)
for _, name := range ctx.StringSlice("names") {
expect(t, name == "john", true)
}

expect(t, len(ctx.IntSlice("u")), 1)
for _, userId := range ctx.IntSlice("userIds") {
expect(t, userId == 3, true)
}

expect(t, len(ctx.Int64Slice("p")), 1)
for _, phoneNumber := range ctx.Int64Slice("phoneNumbers") {
expect(t, phoneNumber == 123456789, true)
}
return nil
},
},
},
{
args: []string{"", "-n", "jane", "-n", "bob", "-u", "5", "-u", "10", "-p", "987654321"},
app: &App{
Flags: []Flag{
StringSliceFlag{Name: "names, n", Value: &StringSlice{"john"}},
IntSliceFlag{Name: "userIds, u", Value: &IntSlice{3}},
Int64SliceFlag{Name: "phoneNumbers, p", Value: &Int64Slice{123456789}},
},
Action: func(ctx *Context) error {
expect(t, len(ctx.StringSlice("n")), 2)
for _, name := range ctx.StringSlice("names") {
expect(t, name != "john", true)
}

expect(t, len(ctx.IntSlice("u")), 2)
for _, userId := range ctx.IntSlice("userIds") {
expect(t, userId != 3, true)
}

expect(t, len(ctx.Int64Slice("p")), 1)
for _, phoneNumber := range ctx.Int64Slice("phoneNumbers") {
expect(t, phoneNumber != 123456789, true)
}
return nil
},
},
},
{
args: []string{"", "--names", "john", "--userIds", "3", "--phoneNumbers", "123456789"},
app: &App{
Flags: []Flag{
StringSliceFlag{Name: "names, n", Value: &StringSlice{"john"}},
IntSliceFlag{Name: "userIds, u", Value: &IntSlice{3}},
Int64SliceFlag{Name: "phoneNumbers, p", Value: &Int64Slice{123456789}},
},
Action: func(ctx *Context) error {
expect(t, len(ctx.StringSlice("n")), 1)
for _, name := range ctx.StringSlice("names") {
expect(t, name == "john", true)
}

expect(t, len(ctx.IntSlice("u")), 1)
for _, userId := range ctx.IntSlice("userIds") {
expect(t, userId == 3, true)
}

expect(t, len(ctx.Int64Slice("p")), 1)
for _, phoneNumber := range ctx.Int64Slice("phoneNumbers") {
expect(t, phoneNumber == 123456789, true)
}
return nil
},
},
},
}

for _, tt := range tests {
_ = tt.app.Run(tt.args)
}
}