pflag
pflag copied to clipboard
spf13
Fix defaultIsZeroValue check for generic Value types
The defaultIsZeroValue function does not check on the DefValue field, for generic Value types., but instead wrongly checks on the flag Value field.
This results in the help screen wrongly displays the default value for a custom Value option, even though the default value is a "zerovalue".
Reproduce
hellocmd --my-option "some-value" -h
Should not display the default value in the help screen, but does anyways without this patch.
Example code to reproduce
package main
import (
"fmt"
"github.com/spf13/cobra"
"os"
)
var myOption StringOption
func init() {
Command.Flags().Var(&myOption, "my-option", "My custom option")
}
func main() {
if err := Command.Execute(); err != nil {
fmt.Println(err)
os.Exit(1)
}
}
var Command = &cobra.Command{
Use: "hellocmd",
Short: "Hello ",
Long: "Hello long",
Run: func(cmd *cobra.Command, args []string) {
},
}
type StringOption struct {
value string
}
func (s *StringOption) Type() string {
return "string"
}
func (s *StringOption) String() string {
return s.value
}
func (s *StringOption) Set(s2 string) error {
s.value = s2
return nil
}
Similar / related PR
https://github.com/spf13/pflag/pull/361
Is related, however solves this problem specifically for isBoolFlag()=true custom Value types.
While this additionally solves for other custom Value types.
I completely agree. However I am quite new to go, and have no experience with writing tests in go 😅.
I am however willing to give it a try, and learn along the way.
I would appreciate some pointers if you have any to give 😃.
I presume that I should put the test in the existing flag_test.go file?
However this test would require some boilerplate such as a custom implementation of Value type, should this also be in the same file?
Yes, tests for this go in flag_test.go! Here are the official getting-started docs for testing in Go in case you need a reference.
I imagine you should be able to extend this test to ensure your change actually fixes the issue; in other words, with a change to that test but without the feature change in this PR, the test should fail - then applying this PR change again should make the test green again.
I have extended the TestPrintDefault to also check for this edge case.
I also took the liberty to improve the readability of the error output of the test, as I found it quite difficult to read. This however can be reverted, if needed 😃.