Code review comment for lp:~dave-cheney/gnuflag/001-parse-more-gnu-args

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 -=.

« Back to merge proposal