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

Proposed by Dave Cheney
Status: Needs review
Proposed branch: lp:~dave-cheney/gnuflag/001-parse-more-gnu-args
Merge into: lp:~gophers/gnuflag/trunk
Diff against target: 66 lines (+12/-4)
2 files modified
flag.go (+6/-2)
flag_test.go (+6/-2)
To merge this branch: bzr merge lp:~dave-cheney/gnuflag/001-parse-more-gnu-args
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+127419@code.launchpad.net

Description of the change

add --format=json style gnu args parsing

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

To post a comment you must log in.
Revision history for this message
Dave Cheney (dave-cheney) wrote :

Please take a look.

13. By Dave Cheney

moar test

Revision history for this message
Roger Peppe (rogpeppe) wrote :

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/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

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

moar test

12. By Dave Cheney

support --format=json style gnu args

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'flag.go'
2--- flag.go 2012-02-02 11:25:56 +0000
3+++ flag.go 2012-10-02 06:59:20 +0000
4@@ -35,6 +35,7 @@
5 "os"
6 "sort"
7 "strconv"
8+ "strings"
9 "time"
10 "unicode/utf8"
11 )
12@@ -723,8 +724,11 @@
13
14 // long flag signified with "--" prefix
15 if a[1] == '-' {
16- // TODO allow '=' so we can set bools to false?
17- flagName = a[2:]
18+ v := strings.SplitN(a[2:], "=", 2)
19+ flagName = v[0]
20+ if len(v) > 1 && len(v[1]) > 0 {
21+ f.procFlag = v[1]
22+ }
23 f.procArgs = f.procArgs[1:]
24 return
25 }
26
27=== modified file 'flag_test.go'
28--- flag_test.go 2012-02-02 11:13:43 +0000
29+++ flag_test.go 2012-10-02 06:59:20 +0000
30@@ -119,10 +119,12 @@
31 []string{
32 "--bool2",
33 "--int", "22",
34+ "--int2=9000",
35 "--int64", "0x23",
36 "--uint", "24",
37 "--uint64", "25",
38 "--string", "hello",
39+ "--format=json",
40 "--float64", "2718e28",
41 "--duration", "2m",
42 "one - extra - argument",
43@@ -131,10 +133,12 @@
44 "bool": false,
45 "bool2": true,
46 "int": 22,
47+ "int2": 9000,
48 "int64": int64(0x23),
49 "uint": uint(24),
50 "uint64": uint64(25),
51 "string": "hello",
52+ "format": "json",
53 "float64": 2718e28,
54 "duration": 2 * 60 * time.Second,
55 },
56@@ -369,8 +373,8 @@
57 var buf bytes.Buffer
58 flags.SetOutput(&buf)
59 flags.Init("test", ContinueOnError)
60- flags.Parse(true, []string{"-unknown"})
61- if out := buf.String(); !strings.Contains(out, "-unknown") {
62+ flags.Parse(true, []string{"--unknown"})
63+ if out := buf.String(); !strings.Contains(out, "unknown") {
64 t.Logf("expected output mentioning unknown; got %q", out)
65 }
66 }

Subscribers

People subscribed via source and target branches