Merge lp:~rogpeppe/gnuflag/support-equals into lp:~gophers/gnuflag/trunk

[Roger Peppe]

gnuflag: implement = in flags.

We should be pretty close to compatible with
all GNU flag usage now.

https://codereview.appspot.com/6598052/

[Roger Peppe (rogpeppe)]

Reviewers: mp+127469_code.launchpad.net,

Message:
Please take a look.

Description:
gnuflag: implement = in flags.

https://code.launchpad.net/~rogpeppe/gnuflag/support-equals/+merge/127469

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6598052/

Affected files:
   A [revision details]
   M flag.go
   M flag_test.go

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6598052/

[lp:~rogpeppe/gnuflag/support-equals]

14. By Roger Peppe on 2012-10-02

refine behaviour of -x=foo

[Roger Peppe (rogpeppe)]

i've changed it now, based on your remarks from your original CL.

-x=foo is different from how i imagined it, but
i think it's better to go with gnu-faithfullness in this case.

PTAL.

On 2 October 2012 14:16, <email address hidden> wrote:
> On 2012/10/02 12:27:23, rog wrote:
>>
>> Please take a look.
>
>
> LGTM, nice tests. Thanks.
>
> https://codereview.appspot.com/6598052/

[Roger Peppe (rogpeppe)]

*** Submitted:

gnuflag: implement = in flags.

We should be pretty close to compatible with
all GNU flag usage now.

R=dfc
CC=
https://codereview.appspot.com/6598052

https://codereview.appspot.com/6598052/

=== modified file 'flag.go'
--- flag.go	2012-02-02 11:25:56 +0000
+++ flag.go	2012-10-02 14:10:24 +0000
@@ -35,6 +35,7 @@
 	"os"
 	"sort"
 	"strconv"
+	"strings"
 	"time"
 	"unicode/utf8"
 )
@@ -688,16 +689,18 @@
 	}
 }
 
-func (f *FlagSet) parseOneGnu() (flagName string, finished bool, err error) {
+func (f *FlagSet) parseOneGnu() (flagName string, long, finished bool, err error) {
 	if len(f.procArgs) == 0 {
-		return "", true, nil
+		finished = true
+		return
 	}
 
 	// processing previously encountered single-rune flag
 	if flag := f.procFlag; len(flag) > 0 {
 		_, n := utf8.DecodeRuneInString(flag)
 		f.procFlag = flag[n:]
-		return flag[0:n], false, nil
+		flagName = flag[0:n]
+		return
 	}
 
 	a := f.procArgs[0]
@@ -707,25 +710,38 @@
 		if f.allowIntersperse {
 			f.args = append(f.args, a)
 			f.procArgs = f.procArgs[1:]
-			return "", false, nil
+			return
 		}
 		f.args = append(f.args, f.procArgs...)
 		f.procArgs = nil
-		return "", true, nil
+		finished = true
+		return
 	}
 
 	// end of flags
 	if f.procArgs[0] == "--" {
 		f.args = append(f.args, f.procArgs[1:]...)
 		f.procArgs = nil
-		return "", true, nil
+		finished = true
+		return
 	}
 
 	// long flag signified with "--" prefix
 	if a[1] == '-' {
-		// TODO allow '=' so we can set bools to false?
-		flagName = a[2:]
+		long = true
+		i := strings.Index(a, "=")
+		if i < 0 {
+			f.procArgs = f.procArgs[1:]
+			flagName = a[2:]
+			return
+		}
+		flagName = a[2:i]
+		if flagName == "" {
+			err = fmt.Errorf("empty flag in argument %q", a)
+			return
+		}
 		f.procArgs = f.procArgs[1:]
+		f.procFlag = a[i:]
 		return
 	}
 
@@ -745,7 +761,7 @@
 	return "-" + name
 }
 
-func (f *FlagSet) parseGnuFlagArg(name string) (finished bool, err error) {
+func (f *FlagSet) parseGnuFlagArg(name string, long bool) (finished bool, err error) {
 	m := f.formal
 	flag, alreadythere := m[name] // BUG
 	if !alreadythere {
@@ -756,7 +772,9 @@
 		// TODO print --xxx when flag is more than one rune.
 		return false, f.failf("flag provided but not defined: %s", flagWithMinus(name))
 	}
-	if fv, ok := flag.Value.(*boolValue); ok { // special case: doesn't need an arg
+	if fv, ok := flag.Value.(*boolValue); ok && !strings.HasPrefix(f.procFlag, "=") {
+		// special case: doesn't need an arg, and an arg hasn't
+		// been provided explicitly.
 		fv.Set("true")
 	} else {
 		// It must have a value, which might be the next argument.
@@ -765,6 +783,12 @@
 		if f.procFlag != "" {
 			// value directly follows flag
 			value = f.procFlag
+			if long {
+				if value[0] != '=' {
+					panic("no leading '=' in long flag")
+				}
+				value = value[1:]
+			}
 			hasValue = true
 			f.procFlag = ""
 		}
@@ -800,10 +824,10 @@
 	f.args = nil
 	f.allowIntersperse = allowIntersperse
 	for {
-		name, finished, err := f.parseOneGnu()
+		name, long, finished, err := f.parseOneGnu()
 		if !finished {
 			if name != "" {
-				finished, err = f.parseGnuFlagArg(name)
+				finished, err = f.parseGnuFlagArg(name, long)
 			}
 		}
 		if err != nil {

=== modified file 'flag_test.go'
--- flag_test.go	2012-02-02 11:13:43 +0000
+++ flag_test.go	2012-10-02 14:10:24 +0000
@@ -108,168 +108,228 @@
 }
 
 var parseTests = []struct {
+	about       string
 	intersperse bool
 	args        []string
 	vals        map[string]interface{}
 	remaining   []string
 	error       string
-}{
-	{
-		true,
-		[]string{
-			"--bool2",
-			"--int", "22",
-			"--int64", "0x23",
-			"--uint", "24",
-			"--uint64", "25",
-			"--string", "hello",
-			"--float64", "2718e28",
-			"--duration", "2m",
-			"one - extra - argument",
-		},
-		map[string]interface{}{
-			"bool":     false,
-			"bool2":    true,
-			"int":      22,
-			"int64":    int64(0x23),
-			"uint":     uint(24),
-			"uint64":   uint64(25),
-			"string":   "hello",
-			"float64":  2718e28,
-			"duration": 2 * 60 * time.Second,
-		},
-		[]string{
-			"one - extra - argument",
-		},
-		"",
-	},
-	{
-		true,
-		[]string{
-			"-a",
-			"-",
-			"-bc",
-			"2",
-			"-de1s",
-			"-f2s",
-			"-g", "3s",
-			"--h",
-			"--long",
-			"--long2", "-4s",
-			"3",
-			"4",
-			"--", "-5",
-		},
-		map[string]interface{}{
-			"a":     true,
-			"b":     true,
-			"c":     true,
-			"d":     true,
-			"e":     "1s",
-			"f":     "2s",
-			"g":     "3s",
-			"h":     true,
-			"long":  true,
-			"long2": "-4s",
-			"z":     "default",
-			"www":   99,
-		},
-		[]string{
-			"-",
-			"2",
-			"3",
-			"4",
-			"-5",
-		},
-		"",
-	}, {
-		true,
-		[]string{
-			"-a",
-			"--",
-			"-b",
-		},
-		map[string]interface{}{
-			"a": true,
-			"b": false,
-		},
-		[]string{
-			"-b",
-		},
-		"",
-	}, {
-		false,
-		[]string{
-			"-a",
-			"foo",
-			"-b",
-		},
-		map[string]interface{}{
-			"a": true,
-			"b": false,
-		},
-		[]string{
-			"foo",
-			"-b",
-		},
-		"",
-	},
-	{
-		false,
-		[]string{
-			"-a",
-			"--",
-			"foo",
-			"-b",
-		},
-		map[string]interface{}{
-			"a": true,
-			"b": false,
-		},
-		[]string{
-			"foo",
-			"-b",
-		},
-		"",
-	},
-	{
-		true,
-		[]string{
-			"-a",
-			"-b",
-		},
-		map[string]interface{}{
-			"a": true,
-		},
-		nil,
-		"flag provided but not defined: -b",
-	},
-	{
-		true,
-		[]string{
-			"-a",
-		},
-		map[string]interface{}{
-			"a": "default",
-		},
-		nil,
-		"flag needs an argument: -a",
-	},
-	{
-		true,
-		[]string{
-			"-a", "b",
-		},
-		map[string]interface{}{
-			"a": 0,
-		},
-		nil,
-		`invalid value "b" for flag -a: strconv.ParseInt: parsing "b": invalid syntax`,
-	},
+}{{
+	about:       "regular args",
+	intersperse: true,
+	args: []string{
+		"--bool2",
+		"--int", "22",
+		"--int64", "0x23",
+		"--uint", "24",
+		"--uint64", "25",
+		"--string", "hello",
+		"--float64", "2718e28",
+		"--duration", "2m",
+		"one - extra - argument",
+	},
+	vals: map[string]interface{}{
+		"bool":     false,
+		"bool2":    true,
+		"int":      22,
+		"int64":    int64(0x23),
+		"uint":     uint(24),
+		"uint64":   uint64(25),
+		"string":   "hello",
+		"float64":  2718e28,
+		"duration": 2 * 60 * time.Second,
+	},
+	remaining: []string{
+		"one - extra - argument",
+	},
+}, {
+	about:       "playing with -",
+	intersperse: true,
+	args: []string{
+		"-a",
+		"-",
+		"-bc",
+		"2",
+		"-de1s",
+		"-f2s",
+		"-g", "3s",
+		"--h",
+		"--long",
+		"--long2", "-4s",
+		"3",
+		"4",
+		"--", "-5",
+	},
+	vals: map[string]interface{}{
+		"a":     true,
+		"b":     true,
+		"c":     true,
+		"d":     true,
+		"e":     "1s",
+		"f":     "2s",
+		"g":     "3s",
+		"h":     true,
+		"long":  true,
+		"long2": "-4s",
+		"z":     "default",
+		"www":   99,
+	},
+	remaining: []string{
+		"-",
+		"2",
+		"3",
+		"4",
+		"-5",
+	},
+}, {
+	about:       "flag after explicit --",
+	intersperse: true,
+	args: []string{
+		"-a",
+		"--",
+		"-b",
+	},
+	vals: map[string]interface{}{
+		"a": true,
+		"b": false,
+	},
+	remaining: []string{
+		"-b",
+	},
+}, {
+	about: "flag after end",
+	args: []string{
+		"-a",
+		"foo",
+		"-b",
+	},
+	vals: map[string]interface{}{
+		"a": true,
+		"b": false,
+	},
+	remaining: []string{
+		"foo",
+		"-b",
+	},
+}, {
+	about: "arg and flag after explicit end",
+	args: []string{
+		"-a",
+		"--",
+		"foo",
+		"-b",
+	},
+	vals: map[string]interface{}{
+		"a": true,
+		"b": false,
+	},
+	remaining: []string{
+		"foo",
+		"-b",
+	},
+}, {
+	about: "boolean args, explicitly and non-explicitly given",
+	args: []string{
+		"--a=false",
+		"--b=true",
+		"--c",
+	},
+	vals: map[string]interface{}{
+		"a": false,
+		"b": true,
+		"c": true,
+	},
+}, {
+	about: "using =",
+	args: []string{
+		"--arble=bar",
+		"--bletch=",
+		"--a=something",
+		"-b=other",
+		"-cdand more",
+		"--curdle=--milk",
+		"--sandwich", "=",
+		"--darn=",
+		"=arg",
+	},
+	vals: map[string]interface{}{
+		"arble":    "bar",
+		"bletch":   "",
+		"a":        "something",
+		"b":        "=other",
+		"c":        true,
+		"d":        "and more",
+		"curdle":   "--milk",
+		"sandwich": "=",
+		"darn":     "",
+	},
+	remaining: []string{"=arg"},
+}, {
+	about: "empty flag #1",
+	args: []string{
+		"--=bar",
+	},
+	error: `empty flag in argument "--=bar"`,
+}, {
+	about: "single-letter equals",
+	args: []string{
+		"-=bar",
+	},
+	error: `flag provided but not defined: -=`,
+}, {
+	about: "empty flag #2",
+	args: []string{
+		"--=",
+	},
+	error: `empty flag in argument "--="`,
+}, {
+	about: "no equals",
+	args: []string{
+		"-=",
+	},
+	error: `flag provided but not defined: -=`,
+}, {
+	args: []string{
+		"-a=true",
+	},
+	vals: map[string]interface{}{
+		"a": true,
+	},
+	error: `invalid value "=true" for flag -a: strconv.ParseBool: parsing "=true": invalid syntax`,
+}, {
+	intersperse: true,
+	args: []string{
+		"-a",
+		"-b",
+	},
+	vals: map[string]interface{}{
+		"a": true,
+	},
+	error: "flag provided but not defined: -b",
+}, {
+	intersperse: true,
+	args: []string{
+		"-a",
+	},
+	vals: map[string]interface{}{
+		"a": "default",
+	},
+	error: "flag needs an argument: -a",
+}, {
+	intersperse: true,
+	args: []string{
+		"-a", "b",
+	},
+	vals: map[string]interface{}{
+		"a": 0,
+	},
+	error: `invalid value "b" for flag -a: strconv.ParseInt: parsing "b": invalid syntax`,
+},
 }
 
 func testParse(newFlagSet func() *FlagSet, t *testing.T) {
 	for i, g := range parseTests {
+		t.Logf("test %d. %s", i, g.about)
 		f := newFlagSet()
 		flags := make(map[string]interface{})
 		for name, val := range g.vals {
@@ -291,28 +351,30 @@
 			case time.Duration:
 				flags[name] = f.Duration(name, 5*time.Second, "duration value")
 			default:
-				panic(fmt.Errorf("unhandled type %T", val))
+				t.Fatalf("unhandled type %T", val)
 			}
 		}
 		err := f.Parse(g.intersperse, g.args)
-		if err != nil {
-			if err.Error() != g.error {
-				t.Errorf("test %d; expected error %q got %q", i, g.error, err.Error())
+		if g.error != "" {
+			if err == nil {
+				t.Errorf("expected error %q got nil", g.error)
+			} else if err.Error() != g.error {
+				t.Errorf("expected error %q got %q", g.error, err.Error())
 			}
 			continue
 		}
 		for name, val := range g.vals {
 			actual := reflect.ValueOf(flags[name]).Elem().Interface()
 			if val != actual {
-				t.Errorf("test %d: flag %q, expected %v got %v", i, name, val, actual)
+				t.Errorf("flag %q, expected %v got %v", name, val, actual)
 			}
 		}
 		if len(f.Args()) != len(g.remaining) {
-			t.Fatalf("test %d: remaining args, expected %q got %q", i, g.remaining, f.Args())
+			t.Fatalf("remaining args, expected %q got %q", g.remaining, f.Args())
 		}
 		for j, a := range f.Args() {
 			if a != g.remaining[j] {
-				t.Errorf("test %d: arg %d, expected %q got %q", i, j, g.remaining[i], a)
+				t.Errorf("arg %d, expected %q got %q", j, g.remaining[i], a)
 			}
 		}
 	}