Fix:(issue_2066) Remove dependency on golang flag library #2074

dearchap · 2025-03-10T19:46:36Z

What type of PR is this?

(REQUIRED)

cleanup
feature

What this PR does / why we need it:

(REQUIRED)

Remove dependence of this library on golang's flag library. We are being constrained by parsing capabilities of golang's flag library and have to create workarounds. This PR does away with all workarounds and does the parsing of the arguments from scratch. The command code has been split into multiple files

command_run.go : the start point of command.Run
command_setup.go : everything related to setting up defaults for a Command instance
command_parse.go : the heart of the parsing algorithm

All prior workaround from parse.go have been folded into command_parse.go. The BoolWithInverseFlag has been rewritten from scratch to do away with hacky behaviour of using a BoolFlag underneath. All tests have been updated to remove flag.FlagSet requirement. The public interface for Flags has been kept the same with the exception of addition of lifecycle functions for the Flag interface. (TBD. should those be moved into a separate Lifecycle interface)

Which issue(s) this PR fixes:

(REQUIRED)

Fixes #2066
Fixes #2078

Special notes for your reviewer:

(fill-in or delete this section)

Testing

(fill-in or delete this section)

go test -run=.

Release Notes

(REQUIRED)

abitrolly · 2025-03-17T13:57:25Z

We are being constrained by parsing capabilities of golang's flag library and have to create workarounds. This PR does away with all workarounds and does the parsing of the arguments from scratch.

Hi @dearchap. It would be useful to document all the bad sides, because eventually it may improve upstream state of things for everybody. Like the fresh story here https://www.dolthub.com/blog/2025-03-07-archiving-the-dolthub-swiss-github-repository/ We just need one page to do that.

TBH I am puzzled what's wrong with Go flag and I don't see this PR removing the import.

dearchap · 2025-03-19T00:03:54Z

We are being constrained by parsing capabilities of golang's flag library and have to create workarounds. This PR does away with all workarounds and does the parsing of the arguments from scratch.

Hi @dearchap. It would be useful to document all the bad sides, because eventually it may improve upstream state of things for everybody. Like the fresh story here https://www.dolthub.com/blog/2025-03-07-archiving-the-dolthub-swiss-github-repository/ We just need one page to do that.

TBH I am puzzled what's wrong with Go flag and I don't see this PR removing the import.

@abitrolly There is no issue itself with golang flag. However its parsing is not exposed so we have had to workaround to support short options, skipping flag parsing, positional arguments, detect underlying errors based on error string and so on. This gives us an opportunity to get rid of it altogether. The flag imports are still needed since we support ExtFlags i.e flags defined in the golang flag libary can co-exist with cli flag.

System233 · 2025-03-23T09:10:18Z

Thanks for your work! Since this PR is too large for me to review in its entirety.
Here are some of my test cases, hope this helps!

My expectations are as follows:

When -- appears in the input arguments and there are no positional arguments before it, args should store only the arguments after --.
```
test --flag x -- a b c  # => args=[a b c], flag=x
```

When there are positional arguments before --, args should store all positional arguments starting from the first one (excluding flags).

test x1 --flag x a b c  # => args=[x1 a b c], flag=x
test x1 --flag x -- a b c  # => args=[x1 -- a b c], flag=x
test x1 --bool-flag -- a b c  # args=[x1 -- a b c], boolFlag=true
test x1 --string-flag -- a b c  # error!

When there are positional arguments and -- appears, args should store all positional arguments from the first one up to -- (excluding flags). Flags should be parsed normally before --, but flag parsing should stop after --, and args should include --.
```
test x1 -- a b c --flag val  # => args=[x1 -- a b c --flag val], flag=
test x1 --flag x -- a b c --flag val  # => args=[x1 -- a b c --flag val], flag=x
```

In summary: If -- first appears as a positional argument, it should be interpreted as an args separator and removed. Otherwise, it should be preserved. Optionally, a configuration flag could be provided based on community feedback.
The behavior of -- should ideally remain consistent with v2.

Regarding array-type flag inputs, I am unsure whether v3 still supports a delimiter option (as the documentation does not mention it). My primary concern is fixing the related issues in v2. Additionally, one very important requirement is the ability to configure the subcommand separator—either by exposing an API for it or by simplifying the configuration while keeping it consistent with the current declarative API.

As for the SkipFlagParsing option, I think it can be retained, as it has some use in subcommands.

More Test cases

`TestStringFlagTerminator`

Name	Input	Expected `--flag` value	Expected args	error?	Expected error message
flag and args after terminator	`["test", "--flag", "x", "--", "test", "a1", "a2", "a3"]`	`"x"`	`["test", "a1", "a2", "a3"]`	No	-
missing flag value due to terminator	`["test", "--flag", "--", "x"]`	-	-	Yes	`"flag needs an argument"`
terminator with no trailing args	`["test", "--flag", "x", "--"]`	`"x"`	`[]`	No	-
no terminator, only flag	`["test", "--flag", "x"]`	`"x"`	`[]`	No	-
flag defined after `--`	`["test", "--", "x", "--flag=value"]`	`""`	`["x", "--flag=value"]`	No	-
flag and without `--`	`["test", "--flag", "value", "x"]`	`"value"`	`["x"]`	No	-

`TestBoolFlagTerminator`

Name	Input	Expected `--flag` value	Expected args	error?	Expected error message
bool flag with invalid non-bool value	`["test", "--flag", "x", "--", "test", "a1", "a2", "a3"]`	-	-	Yes	`"invalid syntax"`
bool flag omitted value defaults to true	`["test", "--flag", "--", "x"]`	`true`	`["x"]`	No	-
bool flag explicitly set to false	`["test", "--flag=false", "--", "x"]`	`false`	`["x"]`	No	-
bool flag defined after `--`	`["test", "--", "x", "--flag=true"]`	`false`	`["x", "--flag=true"]`	No	-
bool flag and without `--`	`["test", "--flag=true", "x"]`	`true`	`["x"]`	No	-

`TestSliceStringFlagParsing`

Name	Input	Expected `--flag` value	Expected args	error?	Expected error message
single value without delimiter (no split)	`cmdNoDelimiter`	`["test", "--flag", "x"]`	`["x"]`	No	-
multiple values with comma (default split)	`cmdNoDelimiter`	`["test", "--flag", "x,y"]`	`["x", "y"]`	No	-
without delimiter specified, value remains unsplit	`cmdNoDelimiter`	`["test", "--flag", "x:y"]`	`["x:y"]`	No	-

Code

I had an AI generate a unit test based on my provided test cases and ran it briefly. It appears that in the latest PR version, -- is still always retained in args, requiring users to handle it manually.

package cli_test

import (
	"context"
	"strings"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/urfave/cli/v3"
)

// TestStringFlagTerminator tests the string flag "--flag" with "--" terminator.
func TestStringFlagTerminator(t *testing.T) {
	var flagVal string
	var argsVal []string

	// build minimal command with a StringFlag "flag"
	cmd := &cli.Command{
		Name: "test",
		Flags: []cli.Flag{
			&cli.StringFlag{
				Name:        "flag",
				Usage:       "a string flag",
				Destination: &flagVal,
			},
		},
		Action: func(ctx context.Context, c *cli.Command) error {
			argsVal = c.Args().Slice()
			return nil
		},
	}

	tests := []struct {
		name         string
		input        []string
		expectFlag   string
		expectArgs   []string
		expectErr    bool
		errorContain string
	}{
		{
			name:       "flag and args after terminator",
			input:      []string{"test", "--flag", "x", "--", "test", "a1", "a2", "a3"},
			expectFlag: "x",
			expectArgs: []string{"test", "a1", "a2", "a3"},
		},
		{
			name:         "missing flag value due to terminator",
			input:        []string{"test", "--flag", "--", "x"},
			expectErr:    true,
			errorContain: "flag needs an argument",
		},
		{
			name:       "terminator with no trailing args",
			input:      []string{"test", "--flag", "x", "--"},
			expectFlag: "x",
			expectArgs: []string{},
		},
		{
			name:       "no terminator, only flag",
			input:      []string{"test", "--flag", "x"},
			expectFlag: "x",
			expectArgs: []string{},
		},
		{
			name:       "flag defined after --",
			input:      []string{"test", "--", "x", "--flag=value"},
			expectFlag: "",
			expectArgs: []string{"x", "--flag=value"},
		},
		{
			name:       "flag and without --",
			input:      []string{"test", "--flag", "value","x"},
			expectFlag: "value",
			expectArgs: []string{"x"},
		},
	}

	for _, tc := range tests {
		// Reset values
		flagVal = ""
		argsVal = nil

		t.Run(tc.name, func(t *testing.T) {
			err := cmd.Run(context.Background(), tc.input)
			if tc.expectErr {
				assert.Error(t, err)
				if err != nil {
					assert.Contains(t, strings.ToLower(err.Error()), strings.ToLower(tc.errorContain))
				}
			} else {
				assert.NoError(t, err)
				assert.Equal(t, tc.expectFlag, flagVal)
				assert.Equal(t, tc.expectArgs, argsVal)
			}
		})
	}
}

// TestBoolFlagTerminator tests the bool flag 
func TestBoolFlagTerminator(t *testing.T) {
	var flagVal bool
	var argsVal []string

	// build minimal command with a BoolFlag "flag"
	cmd := &cli.Command{
		Name: "test",
		Flags: []cli.Flag{
			&cli.BoolFlag{
				Name:        "flag",
				Usage:       "a bool flag",
				Destination: &flagVal,
			},
		},
		Action: func(ctx context.Context, c *cli.Command) error {
			argsVal = c.Args().Slice()
			return nil
		},
	}

	tests := []struct {
		name         string
		input        []string
		expectFlag   bool
		expectArgs   []string
		expectErr    bool
		errorContain string
	}{
		{
			name:         "bool flag with invalid non-bool value",
			input:        []string{"test", "--flag", "x", "--", "test", "a1", "a2", "a3"},
			expectErr:    true,
			errorContain: "invalid syntax",
		},
		{
			name:       "bool flag omitted value defaults to true",
			input:      []string{"test", "--flag", "--", "x"},
			expectFlag: true,
			expectArgs: []string{"x"},
		},
		{
			name:       "bool flag explicitly set to false",
			input:      []string{"test", "--flag=false", "--", "x"},
			expectFlag: false,
			expectArgs: []string{"x"},
		},
		{
			name:       "bool flag defined after --",
			input:      []string{"test", "--", "x", "--flag=true"},
			expectFlag: false,
			expectArgs: []string{"x", "--flag=true"},
		},
		{
			name:       "bool flag and without --",
			input:      []string{"test", "--flag=true", "x"},
			expectFlag: true,
			expectArgs: []string{"x"},
		},
	}

	for _, tc := range tests {
		// Reset flag and args
		flagVal = false
		argsVal = nil

		t.Run(tc.name, func(t *testing.T) {
			err := cmd.Run(context.Background(), tc.input)
			if tc.expectErr {
				assert.Error(t, err)
				if err != nil {
					assert.Contains(t, strings.ToLower(err.Error()), strings.ToLower(tc.errorContain))
				}
			} else {
				assert.NoError(t, err)
				assert.Equal(t, tc.expectFlag, flagVal)
				assert.Equal(t, tc.expectArgs, argsVal)
			}
		})
	}
}

// TestSliceStringFlagParsing tests the StringSliceFlag
func TestSliceStringFlagParsing(t *testing.T) {
	var sliceVal []string

	cmdNoDelimiter := &cli.Command{
		Name: "test",
		Flags: []cli.Flag{
			&cli.StringSliceFlag{
				Name:  "flag",
				Usage: "a string slice flag without delimiter",
			},
		},
		Action: func(ctx context.Context, c *cli.Command) error {
			sliceVal = c.StringSlice("flag")
			return nil
		},
	}

	/*cmdWithDelimiter := &cli.Command{
		Name: "test",
		Flags: []cli.Flag{
			&cli.StringSliceFlag{
				Name:      "flag",
				Usage:     "a string slice flag with delimiter",
				Delimiter: ':',
			},
		},
		Action: func(ctx context.Context, c *cli.Command) error {
			sliceVal = c.StringSlice("flag")
			return nil
		},
	}*/

	tests := []struct {
		name         string
		cmd          *cli.Command
		input        []string
		expectSlice  []string
		expectErr    bool
		errorContain string
	}{
		{
			name:        "single value without delimiter (no split)",
			cmd:         cmdNoDelimiter,
			input:       []string{"test", "--flag", "x"},
			expectSlice: []string{"x"},
		},
		{
			name:        "multiple values with comma (default split)",
			cmd:         cmdNoDelimiter,
			input:       []string{"test", "--flag", "x,y"},
			expectSlice: []string{"x", "y"},
		},
		/*{
			name:        "Case 10: with delimiter specified ':'",
			cmd:         cmdWithDelimiter,
			input:       []string{"test", "--flag", "x:y"},
			expectSlice: []string{"x", "y"},
		},*/
		{
			name:        "without delimiter specified, value remains unsplit",
			cmd:         cmdNoDelimiter,
			input:       []string{"test", "--flag", "x:y"},
			expectSlice: []string{"x:y"},
		},
	}

	for _, tc := range tests {
		// Reset sliceVal
		sliceVal = nil

		t.Run(tc.name, func(t *testing.T) {
			err := tc.cmd.Run(context.Background(), tc.input)
			if tc.expectErr {
				assert.Error(t, err)
				if err != nil {
					assert.Contains(t, strings.ToLower(err.Error()), strings.ToLower(tc.errorContain))
				}
			} else {
				assert.NoError(t, err)
				assert.Equal(t, tc.expectSlice, sliceVal)
			}
		})
	}
}

Output

--- FAIL: TestStringFlagTerminator (0.00s)
    --- FAIL: TestStringFlagTerminator/flag_and_args_after_terminator (0.00s)
        my_test.go:95: 
                Error Trace:    /workspaces/cli/tests/my_test.go:95
                Error:          Not equal: 
                                expected: []string{"test", "a1", "a2", "a3"}
                                actual  : []string{"--", "test", "a1", "a2", "a3"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,2 +1,3 @@
                                -([]string) (len=4) {
                                +([]string) (len=5) {
                                + (string) (len=2) "--",
                                  (string) (len=4) "test",
                Test:           TestStringFlagTerminator/flag_and_args_after_terminator
    --- FAIL: TestStringFlagTerminator/missing_flag_value_due_to_terminator (0.00s)
        my_test.go:87: 
                Error Trace:    /workspaces/cli/tests/my_test.go:87
                Error:          An error is expected but got nil.
                Test:           TestStringFlagTerminator/missing_flag_value_due_to_terminator
    --- FAIL: TestStringFlagTerminator/terminator_with_no_trailing_args (0.00s)
        my_test.go:95: 
                Error Trace:    /workspaces/cli/tests/my_test.go:95
                Error:          Not equal: 
                                expected: []string{}
                                actual  : []string{"--"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,2 +1,3 @@
                                -([]string) {
                                +([]string) (len=1) {
                                + (string) (len=2) "--"
                                 }
                Test:           TestStringFlagTerminator/terminator_with_no_trailing_args
    --- FAIL: TestStringFlagTerminator/flag_defined_after_-- (0.00s)
        my_test.go:95: 
                Error Trace:    /workspaces/cli/tests/my_test.go:95
                Error:          Not equal: 
                                expected: []string{"x", "--flag=value"}
                                actual  : []string{"--", "x", "--flag=value"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,2 +1,3 @@
                                -([]string) (len=2) {
                                +([]string) (len=3) {
                                + (string) (len=2) "--",
                                  (string) (len=1) "x",
                Test:           TestStringFlagTerminator/flag_defined_after_--
--- FAIL: TestBoolFlagTerminator (0.00s)
    --- FAIL: TestBoolFlagTerminator/bool_flag_with_invalid_non-bool_value (0.00s)
        my_test.go:170: 
                Error Trace:    /workspaces/cli/tests/my_test.go:170
                Error:          An error is expected but got nil.
                Test:           TestBoolFlagTerminator/bool_flag_with_invalid_non-bool_value
    --- FAIL: TestBoolFlagTerminator/bool_flag_omitted_value_defaults_to_true (0.00s)
        my_test.go:177: 
                Error Trace:    /workspaces/cli/tests/my_test.go:177
                Error:          Not equal: 
                                expected: []string{"x"}
                                actual  : []string{"--", "x"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,2 +1,3 @@
                                -([]string) (len=1) {
                                +([]string) (len=2) {
                                + (string) (len=2) "--",
                                  (string) (len=1) "x"
                Test:           TestBoolFlagTerminator/bool_flag_omitted_value_defaults_to_true
    --- FAIL: TestBoolFlagTerminator/bool_flag_explicitly_set_to_false (0.00s)
        my_test.go:177: 
                Error Trace:    /workspaces/cli/tests/my_test.go:177
                Error:          Not equal: 
                                expected: []string{"x"}
                                actual  : []string{"--", "x"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,2 +1,3 @@
                                -([]string) (len=1) {
                                +([]string) (len=2) {
                                + (string) (len=2) "--",
                                  (string) (len=1) "x"
                Test:           TestBoolFlagTerminator/bool_flag_explicitly_set_to_false
    --- FAIL: TestBoolFlagTerminator/bool_flag_defined_after_-- (0.00s)
        my_test.go:177: 
                Error Trace:    /workspaces/cli/tests/my_test.go:177
                Error:          Not equal: 
                                expected: []string{"x", "--flag=true"}
                                actual  : []string{"--", "x", "--flag=true"}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,2 +1,3 @@
                                -([]string) (len=2) {
                                +([]string) (len=3) {
                                + (string) (len=2) "--",
                                  (string) (len=1) "x",
                Test:           TestBoolFlagTerminator/bool_flag_defined_after_--
FAIL
FAIL    command-line-arguments  0.004s
FAIL

dearchap · 2025-03-23T22:04:01Z

@System233 Thank you for the exhaustive test cases and the test code. Much appreciated. I have added it to the test suite and they all pass with this PR changes with 2 exceptions, both very tricky cases

`TestStringFlagTerminator`

Name	Input	Expected `--flag` value	Expected args	error?	Expected error message
missing flag value due to terminator	`["test", "--flag", "--", "x"]`	-	-	Yes	`"flag needs an argument"`

How do we know that the user doesnt want the "--" to be the value for the string flag ?

`TestBoolFlagTerminator`

Name	Input	Expected `--flag` value	Expected args	error?	Expected error message
bool flag with invalid non-bool value	`["test", "--flag", "x", "--", "test", "a1", "a2", "a3"]`	-	-	Yes	`"invalid syntax"`

How do we know the 'x' is not actually an arg that the user wants ?

This issue here has been raised before for example bool flags are not expected to have args and sometime users do type
--boolFlag true and get a cryptic error due to true being parsed as an arg subsequently.

Would love your thoughts on both these cases .

System233 · 2025-03-24T01:37:36Z

I agree with your view. In the first case, treating "--" as a normal string flag value is more logical and easier to implement.

Regarding the boolFlag issue, the main concern is compatibility. Using --bool-flag=true|false or other designs like --no-bool-flag / --disable-bool-flag would be more reliable. However, if --bool-flag true|false was already supported, changing it now requires careful consideration of its impact. Personally, I also prefer that boolFlag should not accept arguments.

dearchap · 2025-03-24T10:40:56Z

This library does not support --bool-flag true|false so I think we can remove these two tests. Thanks a lot for the feedback. I'm going to merge this PR. cc @urfave/cli

dearchap added 2 commits March 4, 2025 17:27

Test

e2c5aed

Fix:(issue_2066) Remove dependency on golang flag

94ba4f7

This was referenced Mar 11, 2025

Unknown flag parsing is broken from v2 #2072

Open

Re-document SkipFlagParsing #2068

Closed

dearchap added 6 commits March 12, 2025 11:48

Fix more tests

4375a9a

Remove dead code

05a38a6

Remove dead code

dc6ec51

Fix tests

3ba2963

Fix tests

061f95c

Add comments

e046264

dearchap marked this pull request as ready for review March 16, 2025 20:16

dearchap requested a review from a team as a code owner March 16, 2025 20:16

dearchap added 4 commits March 16, 2025 16:29

Remove dead code

1f2a1d7

Add more tests

80beee5

Fix more tests

799ee5e

Fix more tests

65b91f1

dearchap added 6 commits March 17, 2025 20:34

Fix more tests

76bd006

Fix lint/format errors

d0e51ff

Fix lint

0224527

run v3approve

fcf7d45

remove dead code

ec54d61

Fix tests

7e664b9

dearchap added 3 commits March 18, 2025 20:04

run v3approve

7df9a4f

Add more tests

6671d4e

Add defer

637922a

dearchap mentioned this pull request Mar 20, 2025

**Terrible CLI Library** Both v2/v3 have serious bugs #2066

Closed

dearchap added 2 commits March 22, 2025 18:45

Simplify code

bc89fbc

Organize mutex tests

bf8e261

dearchap added 3 commits March 22, 2025 19:12

Add mutex group flags to post parse

2fac7e9

Add mutex flags for post parse

a978f67

Run gofmt

32910cb

dearchap added 2 commits March 23, 2025 17:47

Remove -- from arg slice

2e1ec2e

Add more tests

89f3402

Run gofmt

6813d2b

dearchap merged commit 809e4fb into urfave:main Mar 24, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix:(issue_2066) Remove dependency on golang flag library #2074

Fix:(issue_2066) Remove dependency on golang flag library #2074

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix:(issue_2066) Remove dependency on golang flag library #2074

Fix:(issue_2066) Remove dependency on golang flag library #2074

Uh oh!

Conversation

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Testing

Release Notes

Uh oh!

Uh oh!

Uh oh!

More Test cases

TestStringFlagTerminator

TestBoolFlagTerminator

TestSliceStringFlagParsing

Code

Output

Uh oh!

Uh oh!

TestStringFlagTerminator

TestBoolFlagTerminator

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

`TestStringFlagTerminator`

`TestBoolFlagTerminator`

`TestSliceStringFlagParsing`

`TestStringFlagTerminator`

`TestBoolFlagTerminator`