Merge pull request #431 from LeGEC/430-fix-usage-message-for-func-flags

fix usage message for func flags, fix arguments order

Author: tomasaschan

bool_func.go

@@ -5,7 +5,7 @@ type boolfuncValue func(string) error
 
 func (f boolfuncValue) Set(s string) error { return f(s) }
 
-func (f boolfuncValue) Type() string { return "func" }
+func (f boolfuncValue) Type() string { return "boolfunc" }
 
 func (f boolfuncValue) String() string { return "" } // same behavior as stdlib 'flag' package
 
@@ -35,6 +35,6 @@ func BoolFunc(name string, usage string, fn func(string) error) {
 }
 
 // BoolFuncP is like BoolFunc, but accepts a shorthand letter that can be used after a single dash.
-func BoolFuncP(name, shorthand string, fn func(string) error, usage string) {
+func BoolFuncP(name, shorthand string, usage string, fn func(string) error) {
 	CommandLine.BoolFuncP(name, shorthand, usage, fn)
 }

bool_func_test.go

@@ -75,7 +75,7 @@ func TestBoolFuncCompat(t *testing.T) {
 		args := []string{"--bflag", "--bflag=false", "--bflag=1", "--bflag=bar", "--bflag="}
 
 		// It turns out that, even though the function is called "BoolFunc",
-		// the stanard flag package does not try to parse the value assigned to
+		// the standard flag package does not try to parse the value assigned to
 		// that cli flag as a boolean. The string provided on the command line is
 		// passed as is to the callback.
 		//   e.g: with "--bflag=not_a_bool" on the command line, the FlagSet does not
@@ -106,7 +106,7 @@ func TestBoolFuncCompat(t *testing.T) {
 		flagName := "bflag"
 		args := []string{"--bflag", "--bflag=err", "--bflag=after"}
 
-		// test behavior of standard flag.Fset with an error triggere by the callback:
+		// test behavior of standard flag.Fset with an error triggered by the callback:
 		// (note: as can be seen in 'runCase()', if the callback sees "err" as a value
 		//  for the bool flag, it will return an error)
 		stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError)
@@ -145,3 +145,33 @@ func TestBoolFuncCompat(t *testing.T) {
 		}
 	})
 }
+
+func TestBoolFuncUsage(t *testing.T) {
+	t.Run("regular func flag", func(t *testing.T) {
+		// regular boolfunc flag:
+		// expect to see '--flag1' followed by the usageMessage, and no mention of a default value
+		fset := NewFlagSet("unittest", ContinueOnError)
+		fset.BoolFunc("flag1", "usage message", func(s string) error { return nil })
+		usage := fset.FlagUsagesWrapped(80)
+
+		usage = strings.TrimSpace(usage)
+		expected := "--flag1   usage message"
+		if usage != expected {
+			t.Fatalf("unexpected generated usage message\n  expected: %s\n       got: %s", expected, usage)
+		}
+	})
+
+	t.Run("func flag with placeholder name", func(t *testing.T) {
+		// func flag, with a placeholder name:
+		// if usageMesage contains a placeholder, expect '--flag2 {placeholder}'; still expect no mention of a default value
+		fset := NewFlagSet("unittest", ContinueOnError)
+		fset.BoolFunc("flag2", "usage message with `name` placeholder", func(s string) error { return nil })
+		usage := fset.FlagUsagesWrapped(80)
+
+		usage = strings.TrimSpace(usage)
+		expected := "--flag2 name   usage message with name placeholder"
+		if usage != expected {
+			t.Fatalf("unexpected generated usage message\n  expected: %s\n       got: %s", expected, usage)
+		}
+	})
+}

flag.go

@@ -549,7 +549,7 @@ func (f *FlagSet) PrintDefaults() {
 func (f *Flag) defaultIsZeroValue() bool {
 	switch f.Value.(type) {
 	case boolFlag:
-		return f.DefValue == "false"
+		return f.DefValue == "false" || f.DefValue == ""
 	case *durationValue:
 		// Beginning in Go 1.7, duration zero values are "0s"
 		return f.DefValue == "0" || f.DefValue == "0s"
@@ -599,8 +599,10 @@ func UnquoteUsage(flag *Flag) (name string, usage string) {
 
 	name = flag.Value.Type()
 	switch name {
-	case "bool":
+	case "bool", "boolfunc":
 		name = ""
+	case "func":
+		name = "value"
 	case "float64":
 		name = "float"
 	case "int64":
@@ -718,7 +720,7 @@ func (f *FlagSet) FlagUsagesWrapped(cols int) string {
 			switch flag.Value.Type() {
 			case "string":
 				line += fmt.Sprintf("[=\"%s\"]", flag.NoOptDefVal)
-			case "bool":
+			case "bool", "boolfunc":
 				if flag.NoOptDefVal != "true" {
 					line += fmt.Sprintf("[=%s]", flag.NoOptDefVal)
 				}

func.go

@@ -27,11 +27,11 @@ func (f *FlagSet) FuncP(name string, shorthand string, usage string, fn func(str
 //
 // The callback function will be called every time "--{name}={value}" (or equivalent) is
 // parsed on the command line, with "{value}" as an argument.
-func Func(name string, fn func(string) error, usage string) {
+func Func(name string, usage string, fn func(string) error) {
 	CommandLine.FuncP(name, "", usage, fn)
 }
 
 // FuncP is like Func, but accepts a shorthand letter that can be used after a single dash.
-func FuncP(name, shorthand string, fn func(string) error, usage string) {
+func FuncP(name, shorthand string, usage string, fn func(string) error) {
 	CommandLine.FuncP(name, shorthand, usage, fn)
 }

func_test.go

@@ -112,9 +112,9 @@ func TestFuncCompat(t *testing.T) {
 		flagName := "fnflag"
 		args := []string{"--fnflag", "before", "--fnflag", "err", "--fnflag", "after"}
 
-		// test behavior of standard flag.Fset with an error triggere by the callback:
+		// test behavior of standard flag.Fset with an error triggered by the callback:
 		// (note: as can be seen in 'runCase()', if the callback sees "err" as a value
-		//  for the bool flag, it will return an error)
+		//  for the flag, it will return an error)
 		stdFSet := flag.NewFlagSet("std test", flag.ContinueOnError)
 		stdFSet.SetOutput(io.Discard) // suppress output
 
@@ -151,3 +151,33 @@ func TestFuncCompat(t *testing.T) {
 		}
 	})
 }
+
+func TestFuncUsage(t *testing.T) {
+	t.Run("regular func flag", func(t *testing.T) {
+		// regular func flag:
+		// expect to see '--flag1 value' followed by the usageMessage, and no mention of a default value
+		fset := NewFlagSet("unittest", ContinueOnError)
+		fset.Func("flag1", "usage message", func(s string) error { return nil })
+		usage := fset.FlagUsagesWrapped(80)
+
+		usage = strings.TrimSpace(usage)
+		expected := "--flag1 value   usage message"
+		if usage != expected {
+			t.Fatalf("unexpected generated usage message\n  expected: %s\n       got: %s", expected, usage)
+		}
+	})
+
+	t.Run("func flag with placeholder name", func(t *testing.T) {
+		// func flag, with a placeholder name:
+		// if usageMesage contains a placeholder, expect that name; still expect no mention of a default value
+		fset := NewFlagSet("unittest", ContinueOnError)
+		fset.Func("flag2", "usage message with `name` placeholder", func(s string) error { return nil })
+		usage := fset.FlagUsagesWrapped(80)
+
+		usage = strings.TrimSpace(usage)
+		expected := "--flag2 name   usage message with name placeholder"
+		if usage != expected {
+			t.Fatalf("unexpected generated usage message\n  expected: %s\n       got: %s", expected, usage)
+		}
+	})
+}