Questions about parsing of long flags with = in them

[telemachus]

(First, thanks for ff.) I'm using v4, and I have some questions about how ff.FlagSets parse long flags that use =. After looking at the code, I'm not sure whether I'm seeing a bug or just a feature that I didn't expect.

1. Boolean flags that are passed as --flagname= (that is, with a long flag and no value after =) are parsed as true. Based on this comment, you may be doing this deliberately, but I'm inclined to think that this is a bug resulting from how you handle = earlier in parseLongFlag.
2. All other flag types that omit a value after = consume the next arg as their value. This can have very surprising results for users who mistakenly forget a value after =. Consider the following.

	fs := ff.NewFlagSet("myprogram")
	debug := fs.Bool('d', "debug", "log debug information")
	file := fs.String('f', "file", "", "file to frobnicate")

	if err := fs.Parse([]string{"--file=", "--debug="}); err != nil {
		panic(err)
	}

	fmt.Printf("file=%q\n", *file)
        fmt.Printf("debug=%v\n", *debug)
	fmt.Printf("args = %+v", fs.GetArgs())
        // Output:
        // file="--debug="
        // debug=false
        // args = []

By way of comparison, stdlib's flag treats all empty values after = as errors except for string flags. (Presumably because they only return an error if a strconv conversion fails, and there is no conversion in the case of string values. An empty value == "".) This strikes me as the right approach (maybe even string flags should fail here). Leaving out a value is presumably user error, and parsing should fail and stop. (Alternatively, the treatment of empty values with = should be documented.)

If this is a bug, I think I have a fix, and I'm happy to make a PR. Let me know what you think.

[peterbourgon]

Interesting edge cases!

Let me consider your use cases from first principles...

Boolean flags that are passed as --flagname= (that is, with a long flag and no value after =) are parsed as true.

So, all of -d, --debug, --debug= --debug=1 --debug= , etc. are parsed as true. I think this makes intuitive sense to me. Specifically in the case of somecmd --debug= --somethingelse ... I think it makes the most sense that --debug= is interpreted as equivalent to --debug and therefore parsed as true.

I'm not currently sure if this behavior is intentional, or accidental, in terms of the implementation. I hope it's not an accident.

All other flag types that omit a value after = consume the next arg as their value. This can have very surprising results for users who mistakenly forget a value after =.

Definitely surprising!

My intuition is that --file= --debug should be parsed as --file="" --debug, i.e. with an empty string passed to the --file flag. That is, --flag= should be in general treated as setting flag to "". Conversely, I don't immediately see why --file=, without any specific thing after the = except whitespace, should be considered a parse error.

If --file= --debug= sets the file flag to anything other than an empty string, that's IMO a clear bug, and if you want to fix it, I would be in your debt 🙇 Otherwise I'll make a PR myself in a little bit, here.

Thank you for the issue.

[telemachus]

Interesting edge cases!

Full disclosure: I am writing my own flag-parsing library (because clearly Go needs another one), and I came across these questions while h̶e̶a̶v̶i̶l̶y̶ b̶o̶r̶r̶o̶w̶i̶n̶g̶ consulting ff's implementation. So, thanks for that.

Let me consider your use cases from first principles...

Boolean flags that are passed as --flagname= (that is, with a long flag and no value after =) are parsed as true. I think this makes intuitive sense to me.

So, all of -d, --debug, --debug= --debug=1 --debug= , etc. are parsed as true.

I will push back on this briefly, but then stop bugging you about it. When you say "etc.", that feels a little handwavey to me. Some additional details: if there is a value captured on the right-hand side of --debug=<something>, then that value is passed to strconv.ParseBool. At that point whether the result is true, false, or an error is up to strconv.ParseBool. But in the case of --debug=, you don't pass anything to strconv.ParseBool. If you did pass (e.g.) an empty string, there would be an error, rather than the result true. (It's extra surprising to me that --debug= parses as true according to ff while --debug= returns an error. I think you are wrong that an empty space parses as true, but I will triple check later.)

tl;dr - the way you currently handle boolean flags is inconsistent; sometimes you follow the (documented) logic of strconv.ParseBool, but in the case of -debug= you do not. Even if you don't change the current behavior, I think you should document it.

If --file= --debug= sets the file flag to anything other than an empty string, that's IMO a clear bug, and if you want to fix it, I would be in your debt 🙇 Otherwise I'll make a PR myself in a little bit, here.

I will work on a PR this weekend.

Thank you for the issue.

Again, thank you for ff.

[peterbourgon]

I will push back on this briefly ...

You make fair points.

Let's consider a few separate cases.

• mycommand --debug

In this case I think it's clear that we set debug to true, no ambiguity.

• mycommand --debug=

Here we have an equal sign but no subsequent value, and no further characters beyond the equal sign that could possibly be interpreted as a value.

From my perspective, every flag passed at the command line is always a 2-tuple of flag name and assigned value. Passing --whatever means a flag name of whatever and no assigned value. If whatever is a bool flag, then this implies a value of true; otherwise, it implies the presence of the whatever flag, but no specific value. Passing --foo=bar means a flag name of foo and a value of bar. Passing --x="" means a flag name of x and a value of "" i.e. an empty string. Passing --debug= means a flag name of debug and no assigned value, exactly the same as passing --debug.

I understand the perspective that --debug= should fail parsing. But I don't understand what that strictness achieves. Is there any situation where passing --debug= shouldn't be treated as if it were just --debug?

• mycommand --debug=""

Here we clearly see that the debug flag is set to the empty string, which would evaluate to false. All good, I hope.

---

tl;dr - the way you currently handle boolean flags is inconsistent; sometimes you follow the (documented) logic of strconv.ParseBool, but in the case of -debug= you do not. Even if you don't change the current behavior, I think you should document it.

I think this claim relies on the assumption that -debug= should pass an empty string to strconv.ParseBool, rather than treating that arg the same as -debug. Reasonable people can disagree. I agree with you that there is some ambiguity here. If you're up for it, please do update whatever docs to make this ambiguity less ambiguous!

[telemachus]

I understand the perspective that --debug= should fail parsing. But I don't understand what that strictness achieves. Is there any situation where passing --debug= shouldn't be treated as if it were just --debug?

I was assuming that --debug= was (always? almost always?) the result of user error or a programming mistake. That is, in the first case a person might accidentally mean to enter --debug=false, but forget to type false because they got distracted. In the second case, programA produces (e.g.) a cronjob for programB. programA calls someFunction to fill in VALUE in a line like programB --verbose --frobnicate=VALUE. someFunction goes wrong and puts nothing after --frobnicate=. I am probably overthinking things.

When I work on the other issue, I'll see if it makes sense to add anything to the docs. If it does, I'll make a separate PR for that. Thanks.

[tgulacsi]

#139 fixes the issue, and changes the behaviour only for non-boolean flags.
I hope it will be merged soon.