Allows any flag to expect optional arguments

[fabianofranz]

The motivation for this change is to allow any flag or flag type (not only boolean) to support optional arguments as long as they are declared to do so. Example:

git diff --stat[=<width>]

The logic around flags with empty arguments (e.g. --debug) were detached from the boolean type and moved to a separate attribute called Optional. This allows to mark any flag type as supporting optional arguments.

The difference in comparison to not providing the flag at all is that the Changed attribute is set, so it allows to control the difference of not providing the flag or providing but without an argument.

A possible use case would be:

flag.IntVar(&flagvar, "list", 10, "list the number of items")
flag.MarkOptional("list")

So when using the flag users could either provide the number of items to display in the list, or not provide any argument to use the default int value:

deploy           // perform a deployment
deploy --list    // will list 10 deployments (default value)
deploy --list=20 // will list 20 deployments

The behavior for default types were not changed (the only flags that allow empty arguments by default are boolean).

[fabianofranz]

@eparis @spf13 @smarterclayton FYI

[eparis]

Given

flag.IntVar(&flagvar, "list", 10, "list the number of items")
flag.MarkOptional("list")

What's the different between

./deploy --list

and

./deploy

Sort of unrelated:
The bash autocompletion stuff in spf13/cobra has special case code around booleans. It considers them to not be a "two_word_flags" and it doesn't autocomplete to include an =. How do you think I should handle an int flag? Should ./deploy --lis[tab][tab] show --list=, as it does now, or should it show --list and suggest people type another flag?

[fabianofranz]

@eparis Updated the description with an answer and use case to your first question.

[fabianofranz]

@eparis I'd say in autocomplete it should not show the = in any flag explicitly set as Optional (same behavior as booleans today), and show = in everything else (same as everything that's not boolean today).

[fabianofranz]

In fact I think the help should reflect it too: when a flag is set as Optional, the help could be displayed with [], e.g.:

--list[=size]

[eparis]

So I've been doing some thinking. Never a good idea for anyone... Tell me what you think... (And mind you, I have no idea how to make it work sanely)

What if we changed the Value interface somehow, so it had both a default and a no-option default. So in your code above somehow

flag.IntVar(&flagvar, "list", 10, "list the number of items")

becomes

flag.IntVar(&flagvar, "list", 0, "list the number of items")
SetNoOptionDefault("list", 10)

Bools all turn into SetNoOptionDefault($name, True)

What it would mean for the consumer of pflag is that they can just use the list value.

deploy           // flagvar == 0 so just perform a deployment
deploy --list    // flagvar == 10 so list 10 deployments (no option default value)
deploy --list=20 // flagvar==20 so list 20 deployments

But maybe the code on our end isn't worth it just to keep the users of pflag from having to look at changed...

[eparis]

actually, it is probably as easy as doing it as a string in the Flag structure and calling Set() if the flag was called without an argument....

So probably not as hard to code as I first thought....

[fabianofranz]

@eparis I like how it looks, but the main reason I didn't choose to internalize the no-option-default value initially (instead delegating to the user the control of storing and managing that value and just giving a way to check if it was set, through Changed) is that you would have to handle all possible flag types, e.g.: SetNoOptionDefaultInt, SetNoOptionDefaultString, SetNoOptionDefaultBool and so on.

I'm ok with it if you think it's reasonable, but in a first glance it looked a little too much. If we are adding something to every possible type, then we could just add new *Var methods to specifically handle flags with optional arguments, explicitly marking them as optional and providing the no-option-default value at the same time, e.g.:

flag.OptionalIntVar(&flagvar, "list", 0, 10, "list the number of items")

[eparis]

So I drank a lot and did some hacking on this over the weekend. I completely rewrote parseLongArg and parseShortArg. Not because they were wrong, just because I thought I could do better. In some ways I did better, in some ways worse. But that's not the point here.... The point is the default-ing method isn't too hard.

eparis@b783f34

Before this change, given a variable --list with shorthand -l any of the following would be valid

1. deploy --list=20
2. deploy --list 20
3. deploy -l=20
4. deploy -l 20
5. deploy -l20

Now if we define list as optional we lose 2 and 4. And I guess we lose 5. I guess that makes sense and people shouldn't be toooo shocked about it....

[fabianofranz]

Ok, so you are storing NoOptDefVal as a string and relying on the type conversion to convert it. I think it overall is cleaner and I like it better than having multiple SetNoOptionDefault for multiple types. My only concern would be having two different approaches for similar things facing end users: regular defaults are specified in their original types (true, 10, "string", etc); and argument-present-defaults always as strings ("true", "10", "string", etc). Let me think about it.

That said, I think loosing any of the short|long forms is an issue.

[eparis]

#20 is my alternative. Thoughts?

[fabianofranz]

@eparis I do like how it's exposed to users. But I think loosing any form of short or long flags is an issue. If you can assure we still support all forms then I'm good.

[eparis]

If a user sets NoOptDefVal:

Works:

--list=10
--list
-l
-l=10

Does not work

--list 10
-l 10
-l10

[spf13]

So trying to follow along... I think I understand why this feature is valuable and the different approaches..

It seems though with either approach the experience will be different for the user depending on if the feature is enabled or not. This seems weird to me that a call that works today will no longer work (or could possibly not work in the future) because a developer decided to make a flag optional.

As I understand it

With optional params (works):

--list=10
--list
-l
-l=10

Without optional params (works):

--list=10
-l=10
--list 10
-l 10
-l10

I haven't given it enough thought, but I would hope a solution exists that could cover every case that works today.

[eparis]

@spf13 correct. Both approaches have the same limitation.

When a flag has been set as taking an 'optional' argument, the code has no idea if user intended the next word to be the argument to the flag, to be the next flag, or to be some interspersed word we should ignore.

You are right. If developers turn an existing flag into a 'arg optional' flag users may see breakage.

[eparis]

although admittedly we already this rather odd usage around bools.
Works:

--bool=true
--bool
-b=true (fixed today)
-b

Fails:

--bool true
-b true
-btrue

[spf13]

So the bool case makes a lot of sense to me because it's only used to change the current behavior to the opposite behavior. With the case of bool it's a bit surprising that it takes values at all, but I understand that it would want to do that to enable people to be explicit which is typically a good practice when scripting things as it would remain consistent even if the default changes.

Is there a practical use case where you intend to use this feature? I'd like to understand it a bit better. I see why it's nice, but I don't think I fully understand it's value.

We've also strived to be posix compliant so far. I don't claim to be an expert on the subtleties of the POSIX specification, but I believe this feature is in disagreement with guideline 7 (at the bottom).
http://pubs.opengroup.org/onlinepubs/9699919799//basedefs/V1_chap12.html

[fabianofranz]

@eparis @spf13 Wait, that's not correct. This PR (#18) as originally proposed does not break the current behavior or make any difference either the optional arg feature is enabled or not. All these forms work correctly when having an optional arg (testing right now with my real use case):

deploy -L         deploy --list
deploy -L 2       deploy --list 2
deploy -L=2       deploy --list=2
deploy -L2

I agree that the solution must cover every case that works today, the short and long forms with a space between the flag and argument are important and must work.

[eparis]

@fabianofranz so what does deploy -L --some-other-flag do?

[eparis]

@fabianofranz or deploy -lb where b is some boolean shorthand?

[spf13]

just to be thorough, GNU amends the POSIX standards a bit
http://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

I believe it doesn't address optional argument values in it's specification.

[fabianofranz]

@eparis Fair enough, -L --some-other-flag fails. :)

[eparis]

So big holy crap moment for me:

9294901

broke --flag value !

This has basically NEVER worked, and no one noticed!

I'm just stunned...

[eparis]

Try it yourself:

package main

import (
    "fmt"
    "os"

    "github.com/spf13/pflag"
)

var _ = fmt.Fprintf
var o = os.Stdout

func main() {
    var flagvar int
    pflag.IntVar(&flagvar, "flagname", 1234, "help message for flagname")
    pflag.Parse()

    fmt.Fprintf(o, "val=%v\n", flagvar)
}

$ ./test --flagname 1235
flag needs an argument: --flagname
Usage of ./test:
      --flagname=1234: help message for flagname

[eparis]

my fix is below. But since --flag name hasn't worked since like 2012, maybe we don't have a problem ehre....

diff --git a/flag.go b/flag.go
index 9f16733..93222c8 100644
--- a/flag.go
+++ b/flag.go
@@ -530,11 +530,15 @@ func (f *FlagSet) parseLongArg(s string, args []string) (a []string, err error)
        }
        var value string
        if len(split) == 1 {
-               if bv, ok := flag.Value.(boolFlag); !ok || !bv.IsBoolFlag() {
+               if bv, ok := flag.Value.(boolFlag); ok && bv.IsBoolFlag() {
+                       value = "true"
+               } else if len(a) > 0 {
+                       value = a[0]
+                       a = a[1:]
+               } else {
                        err = f.failf("flag needs an argument: %s", s)
                        return
                }
-               value = "true"
        } else {
                value = split[1]
        }

[eparis]

I decided to take a look at what ls does (and how it represents things in the man page). Basically the ls command in linux does what is being described here. Sometimes you can use a space and sometimes you can't...

It makes me feel like we should go with the optional arg stuff.

$ ls --color
$ ls --color=never
$ ls --color never
ls: cannot access never: No such file or directory

       --color[=WHEN]
              colorize the output; WHEN can be 'never', 'auto', or 'always' (the default); more info below

$ ls -T 7
$ ls --tabsize=7
$ ls --tabsize 7
$ ls --tabsize
ls: option '--tabsize' requires an argument
Try 'ls --help' for more information.

       -T, --tabsize=COLS
              assume tab stops at each COLS instead of 8

[eparis]

@thockin since you pointed out that --flag val doesn't work.

[eparis]

I still have my PR version of this open #20