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.

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

Define flags with their default values in newTestContext #1894

Open
kishansagathiya opened this issue Oct 14, 2021 · 1 comment
Open

Define flags with their default values in newTestContext #1894

kishansagathiya opened this issue Oct 14, 2021 · 1 comment

Comments

@kishansagathiya
Copy link
Contributor

Issue summary

The piece of code described below is meant to define different cmd flags with their default values.
However, it defines them with actual value of the flag as its default value.

for i := range values {
switch v := values[i].(type) {
case bool:
set.Bool(flags[i], v, "")
case string:
set.String(flags[i], v, "")
case uint:
set.Uint(flags[i], v, "")
case int64:
set.Int64(flags[i], v, "")
default:
return nil, fmt.Errorf("unexpected cli value type: %T", values[i])
}
}

Right after this is the code, that is meant to actually set the value. But because we have already used the actual values as default values, this codes ends up not having any impact.

for i := range values {
switch v := values[i].(type) {
case bool:
if v {
err := ctx.Set(flags[i], "true")
if err != nil {
return nil, fmt.Errorf("failed to set cli flag: %T", flags[i])
}
} else {
err := ctx.Set(flags[i], "false")
if err != nil {
return nil, fmt.Errorf("failed to set cli flag: %T", flags[i])
}
}
case string:
err := ctx.Set(flags[i], values[i].(string))
if err != nil {
return nil, fmt.Errorf("failed to set cli flag: %T", flags[i])
}
case uint:
err := ctx.Set(flags[i], strconv.Itoa(int(values[i].(uint))))
if err != nil {
return nil, fmt.Errorf("failed to set cli flag: %T", flags[i])
}
case int64:
err := ctx.Set(flags[i], strconv.Itoa(int(values[i].(int64))))
if err != nil {
return nil, fmt.Errorf("failed to set cli flag: %T", flags[i])
}
default:
return nil, fmt.Errorf("unexpected cli value type: %T", values[i])
}
}

In general, this does not create any problems, but it did when I was trying to add a StringSliceFlag. This is because unlike other flags, StringSlice appends values to existing values when we do StringSlice.Set, while others would overwrite it.

What to change:-

A simpler way would be to use default values of each type like false for bool, 0 for int etc. These are not the true default values for the flag, but it would hurt since we are dealing with tests and all the values would be set later.

Other way is to have a map of flag name to default value and use that to set default values.

Other information and links

@LernaJ
Copy link

LernaJ commented Jul 10, 2024

Bug: Don't know if we need to be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants