fix: --long= should not consume the next argument #139

telemachus · 2025-01-17T19:12:47Z

No description provided.

telemachus · 2025-01-17T19:15:05Z

I added a couple of tests to confirm that (1) for a boolean, --long= parses as true, (2) for other flags --long= parses as --long="", and (3) --long= does not consume the next argument from args.

In the parsing code, I tried to minimize changes, but while I was there I updated two comments to reflect that only long flags were at issue, and I changed len(args) <= 0 to len(args) == 0 since I don't think it's ever possible for a slice to have a negative length.

I considered adding a further test (like below) for --long="" with non-string flags, but I wasn't sure whether testing for such a specific error was too fiddly. Let me know if you think more tests are needed.

{
	Name:         "--flt= -a",
	Constructors: []fftest.Constructor{fftest.CoreConstructor},
	Args:         []string{`--flt=`, `-a`},
	Want:         fftest.Vars{WantParseErrorIs: strconv.ErrSyntax},
},

flag_set.go

ccoVeille · 2025-01-17T20:44:33Z

flag_set.go

 			if len(args) > 0 {
 				if _, err := strconv.ParseBool(args[0]); err == nil {
-					value = args[0] // `-b true` or `--foo false` should also work
+					value = args[0] // `--foo false` should also work
 					args = args[1:]
 				}
 			}


There is also something strange with the code (the code that already exists)

Let's assume --foo expects a string

--foo leads to an error
--foo true leads to "true"
--foo=1 leads to "1"
--foo=false leads to "false"
--foo= leads to ""

But if --foo is a boolean

--foo false
--foo whatever
--foo 0
--foo 2
--foo=2

Leads to this

--foo=false
--foo whatever
--foo=false
--foo=true 2
an error

This behavior is strange to me, but I'm unsure how other libraries parsing flags do

The difference between --foo 2 and --foo=2 is...not great. That is probably why flag in Go's standard library restricts the form --flag arg to non-boolean flags.

I would be in favor of that here as well, but I think @peterbourgon wants to keep boolean parsing as is. (Also, that would probably be a very breaking API change now. Don't know how that plays with v4 being in beta.)

@ccoVeille As a side note, flag in the standard library has a parallel inconsistency but for string flags.

fs := flag.NewFlagSet("whatever", flag.ContinueOnError) cfg := struct { config string // ... }{} fs.StringVar(&cfg.config, "config", "", "Use this configuration file") // ...

whatever -config=foo # No error; config is set to "foo" whatever -config foo # No error; config is set to "foo" whatever -config "" # No error; config is set to "" whatever -config="" # No error; config is set to "" whatever -config # Error, namely "flag needs an argument: -config" whatever -config= # No error; config is set to ""

I think that the last two should return the same error, but they do not. I'm guessing this is because the parser would need to do extra work (not much but some) to detect the difference between -config= and -config="". I doubt anything can be done about it now (breaking API change), but I think it was a mistake.

For me, the behavior of the stdlib is fine here. I might look a bit odd, but I'm fine with it.

It has its logic.

About ff lib, I don't know really. You are fixing a bug with the boolean flag after all, so fixing the bug is somehow already breaking something that was broken 😄, by fixing it.

While your fix is fine, I think the issue of removing the random behavior of boolean flag with a parameter should be considered, at least to have a library that behave like other libraries.

I don't think it was intended, I would remove it. But, except that your PR is fine, you are fixing the behavior with --foo= so nothing about --foo 0

So for me it can be merged as is

For record, I randomly found a discussion about the exact same issue with boolean flag

feature request: footgun-proofing --some-boolean-flag false spf13/pflag#410

telemachus · 2025-02-01T19:13:40Z

@tgulacsi I appreciate the support, but I think we just have to wait for the maintainer to get a chance to review the changes and decide what he wants to do. I don't think that outside people approving will make much a difference one way or the other. (Though, that said, I'm happy for the code review!)

peterbourgon · 2025-02-03T23:49:44Z

flag_set_test.go

 		{args: []string{"--help"}, wantX: false, wantY: true, wantErr: ff.ErrHelp},
 		{args: []string{"--xflag", "-h"}, wantX: true, wantY: true, wantErr: ff.ErrHelp},
 		{args: []string{"-y", "--help"}, wantX: false, wantY: false, wantErr: ff.ErrHelp},
+		{args: []string{"--xflag=", "--help"}, wantX: true, wantY: false, wantErr: ff.ErrHelp},


Shouldn't this be

Suggested change

{args: []string{"--xflag=", "--help"}, wantX: true, wantY: false, wantErr: ff.ErrHelp},

{args: []string{"--xflag=", "--help"}, wantX: true, wantY: true, wantErr: ff.ErrHelp},

?

@peterbourgon I see what you mean: yflag := fs.BoolDefault('y', "yflag", true, "another boolean flag"). I didn't notice that yflag defaulted to true. Sorry.

That said, here's something weird: the tests pass without error in all the cases below.

{args: []string{"--xflag=", "--help"}, wantX: true, wantY: true, wantErr: ff.ErrHelp}

{args: []string{"--xflag=", "--help"}, wantX: true, wantY: false, wantErr: ff.ErrHelp}

{args: []string{"--xflag=", "--help"}, wantX: true, wantErr: ff.ErrHelp}

They all pass. I'm very confused. (I updated to wantY: true, but for some reason it doesn't seem to matter.)

If wantErr is non-nil then the wantX and wantY fields aren't evaluated. My original comment was a bit pedantic, in that sense.

Ah, okay. Either way, I clearly need to understand the test structure better.

cbandy · 2025-08-29T13:59:29Z

While reading the conversations around this, I'm unsure if everyone understands what the shell does with quotes before the Go program receives anything.

For example, this is one argument (word) on the command line that contains no quotes: 'asdf'qqq"xyz"''""''gg

This is a small script that prints each of its arguments on a separate line. With it in args.sh plus chmod +x args.sh, the above looks like this:

#!/bin/sh
printf "|%s|\n" "$@"

$ ./args.sh 'asdf'qqq"xyz"''""''gg
|asdfqqqxyzgg|

There are no quotes for Go to parse in the following, either. Notice that:

-config "" arrives as two arguments, 7 bytes then 0 bytes.
-config="" arrives as one argument, 8 bytes ending with =.

$ ./args.sh -config=foo -config foo -config "" -config="" -config -config=
|-config=foo|
|-config|
|foo|
|-config|
||
|-config=|
|-config|
|-config=|

telemachus · 2025-08-29T15:18:03Z

@cbandy I think I understand your point, but to be clear, do you think that affects the main fix here? That is, even given what you show, I think it's a bug for ff to attach a subsequent argument to --long=. To put this another way, regardless of how the shell handles empty strings and quotes, I don't think that ff should treat []string["--long=", "foo"] as if it were []string["--long=foo"]. Do you think I'm wrong?

cbandy · 2025-08-30T01:12:12Z

I don't think that ff should treat []string["--long=", "foo"] as if it were []string["--long=foo"].

Agreed.

cbandy

Tests LGTM.

cbandy · 2025-08-30T01:08:42Z

flag_set.go

-	if value == "" {
+	if eqFound && f.isBoolFlag && value == "" {
+		value = "true" // `--debug=` amounts to `--debug=true`
+	}


I would move this into the switch, if possible.

switch { case hasEquals && f.isBoolFlag && value == "": // Treat --bool= the same as --bool=true value = "true" case hasEquals: // Use the value within this arg as-is. case f.isBoolFlag: // Use the value of the next arg, if it exists and looks related. ... default: // Use the value of the next arg, if it exists. ... }

I'm happy to look at that, but I'm not going to push any further changes until we hear from @peterbourgon. I'd like to know what he's thinking about this PR before changing it further.

ccoVeille reviewed Jan 17, 2025

View reviewed changes

telemachus force-pushed the long-flag-booleans branch from fba0909 to a86c7b7 Compare January 17, 2025 21:54

ccoVeille mentioned this pull request Jan 28, 2025

Allow long flag with equal sign but no args (--foo= --bar=x) #141

Closed

ccoVeille approved these changes Jan 28, 2025

View reviewed changes

tgulacsi mentioned this pull request Jan 30, 2025

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

Open

tgulacsi approved these changes Feb 1, 2025

View reviewed changes

peterbourgon reviewed Feb 3, 2025

View reviewed changes

fix: --long= should not consume the next argument

326b6b0

telemachus force-pushed the long-flag-booleans branch from a86c7b7 to 326b6b0 Compare February 4, 2025 00:22

ccoVeille mentioned this pull request Mar 14, 2025

feature request: footgun-proofing --some-boolean-flag false spf13/pflag#410

Closed

cbandy reviewed Aug 30, 2025

View reviewed changes

	{args: []string{"--xflag=", "--help"}, wantX: true, wantY: false, wantErr: ff.ErrHelp},
	{args: []string{"--xflag=", "--help"}, wantX: true, wantY: true, wantErr: ff.ErrHelp},

fix: --long= should not consume the next argument #139

Are you sure you want to change the base?

fix: --long= should not consume the next argument #139

Uh oh!

Conversation

telemachus commented Jan 17, 2025

Uh oh!

telemachus commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

telemachus commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbandy commented Aug 29, 2025

Uh oh!

telemachus commented Aug 29, 2025

Uh oh!

cbandy commented Aug 30, 2025

Uh oh!

cbandy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

telemachus commented Jan 17, 2025 •

edited

Loading

telemachus commented Feb 1, 2025 •

edited

Loading