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

Proposed by Dave Cheney
Status: Needs review
Proposed branch: lp://staging/~dave-cheney/gnuflag/001-parse-more-gnu-args
Merge into: lp://staging/~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://staging/~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.staging.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
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches