Merge lp:~dave-cheney/gnuflag/001-parse-more-gnu-args into lp:~gophers/gnuflag/trunk

[Dave Cheney]

add --format=json style gnu args parsing

https://codereview.appspot.com/6588060/

[Dave Cheney (dave-cheney)]

Please take a look.

[lp:~dave-cheney/gnuflag/001-parse-more-gnu-args]

13. By Dave Cheney on 2012-10-02

moar test

[Roger Peppe (rogpeppe)]

thanks for working on this - it needed doing!

a few thoughts below.

https://codereview.appspot.com/6588060/diff/1/flag.go
File flag.go (right):

https://codereview.appspot.com/6588060/diff/1/flag.go#newcode729
flag.go:729: if len(v) > 1 && len(v[1]) > 0 {
this seems a bit odd to me. i think it's perfectly ok to have an empty
*value*, but an empty flag is wrong.

i think we need to return an error here if there's an empty flag. if we
store the entire string following the flag (including the leading '=')
inside f.procFlag, then parseGnuFlagArg can know that it's got that
style of flag, and process accordingly.

that way we allow the following cases:
--foo=false
-x=true
--bar=

but not
--=x

i think -= should probably be an error too.

https://codereview.appspot.com/6588060/diff/1/flag_test.go
File flag_test.go (right):

https://codereview.appspot.com/6588060/diff/1/flag_test.go#newcode127
flag_test.go:127: "--format=json",
more test cases here please.

https://codereview.appspot.com/6588060/

[Roger Peppe (rogpeppe)]

See alternative CL at https://codereview.appspot.com/6598052/

On 2 October 2012 12:15, <email address hidden> wrote:
> Thank you for your comments. I have a slight worry about scope creep, we
> only have to support --format=json to fix the current bug in juju.
>
>
>
> https://codereview.appspot.com/6588060/diff/1/flag.go
> File flag.go (right):
>
> https://codereview.appspot.com/6588060/diff/1/flag.go#newcode729
> flag.go:729: if len(v) > 1 && len(v[1]) > 0 {
> On 2012/10/02 07:10:01, rog wrote:
>>
>> this seems a bit odd to me. i think it's perfectly ok to have an empty
>
> *value*,
>>
>> but an empty flag is wrong.
>
>
>> i think we need to return an error here if there's an empty flag. if
>
> we store
>>
>> the entire string following the flag (including the leading '=')
>
> inside
>>
>> f.procFlag, then parseGnuFlagArg can know that it's got that style of
>
> flag, and
>>
>> process accordingly.
>
>
>> that way we allow the following cases:
>> --foo=false
>
>
> yup, will fix.
>
>> -x=true
>
>
> I don't think this is valid gnuarg

Hmm, that's a good point. I should disallow = for single-letter flags.

>> --bar=
>
>
>> but not
>> --=x
>
>
> good point, this has no name.
>
>
>> i think -= should probably be an error too.
>
>
> I think it is an error in the same way -x=true is an error

actually, -x=true is not necessarily an error with GNU flag parsing - it
specifies that -x has the argument "=true".

-=true is always an error though, as is -=.

Unmerged revisions

13. By Dave Cheney on 2012-10-02

moar test

12. By Dave Cheney on 2012-10-02

support --format=json style gnu args

=== modified file 'flag.go'
--- flag.go	2012-02-02 11:25:56 +0000
+++ flag.go	2012-10-02 06:59:20 +0000
@@ -35,6 +35,7 @@
 	"os"
 	"sort"
 	"strconv"
+	"strings"
 	"time"
 	"unicode/utf8"
 )
@@ -723,8 +724,11 @@
 
 	// long flag signified with "--" prefix
 	if a[1] == '-' {
-		// TODO allow '=' so we can set bools to false?
-		flagName = a[2:]
+		v := strings.SplitN(a[2:], "=", 2)
+		flagName = v[0]
+		if len(v) > 1 && len(v[1]) > 0 {
+			f.procFlag = v[1]
+		}
 		f.procArgs = f.procArgs[1:]
 		return
 	}
 
=== modified file 'flag_test.go'
--- flag_test.go	2012-02-02 11:13:43 +0000
+++ flag_test.go	2012-10-02 06:59:20 +0000
@@ -119,10 +119,12 @@
 		[]string{
 			"--bool2",
 			"--int", "22",
+			"--int2=9000",
 			"--int64", "0x23",
 			"--uint", "24",
 			"--uint64", "25",
 			"--string", "hello",
+			"--format=json",
 			"--float64", "2718e28",
 			"--duration", "2m",
 			"one - extra - argument",
@@ -131,10 +133,12 @@
 			"bool":     false,
 			"bool2":    true,
 			"int":      22,
+			"int2":     9000,
 			"int64":    int64(0x23),
 			"uint":     uint(24),
 			"uint64":   uint64(25),
 			"string":   "hello",
+			"format":   "json",
 			"float64":  2718e28,
 			"duration": 2 * 60 * time.Second,
 		},
@@ -369,8 +373,8 @@
 	var buf bytes.Buffer
 	flags.SetOutput(&buf)
 	flags.Init("test", ContinueOnError)
-	flags.Parse(true, []string{"-unknown"})
-	if out := buf.String(); !strings.Contains(out, "-unknown") {
+	flags.Parse(true, []string{"--unknown"})
+	if out := buf.String(); !strings.Contains(out, "unknown") {
 		t.Logf("expected output mentioning unknown; got %q", out)
 	}
 }