Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Questions about parsing of long flags with = in them #138

Open
telemachus opened this issue Jan 13, 2025 · 4 comments
Open

Questions about parsing of long flags with = in them #138

telemachus opened this issue Jan 13, 2025 · 4 comments

Comments

@telemachus
Copy link

telemachus commented Jan 13, 2025

(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
Copy link
Owner

peterbourgon commented Jan 17, 2025

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
Copy link
Author

telemachus commented Jan 17, 2025

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
Copy link
Owner

peterbourgon commented Jan 17, 2025

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants