[bbadour]

Make Go flags added with AllowExtFlags work like flags defined with urfave/cli:

• PreParse only resets a value that differs from its default, so a flag that does not accept its own default, such as klog's -log_backtrace_at, no longer stops the app from starting.
• Bool flags can be given without a value.
• They show in help with the same placeholder as the flag package, also when other flags have a category.
• Subcommands accept them.
• A Go flag is not added when a flag of the command already has its name, as that flag wins.

What type of PR is this?

• feature

What this PR does / why we need it:

Solve:

• cryptic error messages like "syntax error: expected flag.go:234"
• errors requiring external boolean flags have a value when used
• avoidable astonishment when AllowExtFlags flags behave differently from identical flags defined using urfave/cli

Which issue(s) this PR fixes:

Fixes #2225

Testing

New tests in flag_ext_test.go cover each change. Also tested with apps using klog v2.140.0 and glog v1.0.0, which fail to start on main.

Release Notes

Go flags added with `AllowExtFlags` now work like other flags: an app using klog no longer fails to start, the flags show in help and work in subcommands, and bool flags need no value. Turn a bool flag off with `--flag=false`, as `--flag false` now sets it and passes `false` as an argument.

[bbadour]

Migrating some code from v1, I found this could be less astonishing if, instead, it were:

        if e == nil || e.f == nil || e.f.Value == nil {
                return false
        }
        return e.f.Value.String() != e.f.DefValue

[dearchap]

Actually you can set a flag in the "extFlag.Set()" method and check that here.

[dearchap]

Suggested change

	if err := e.Set("", e.f.DefValue); err != nil && e.f.Value.(flag.Getter).Get() != nil {
	if err := e.Set("", e.f.DefValue); err != nil {

do you need the Get() != nil check ?

[dearchap]

is this dependent on the DefaultValue ? similar to flag library?

[dearchap]

you dont need to define this method. interface casting will check if Flag is an ActionableFlag and call accordingly

[dearchap]

@bbadour Can you add some more tests ?

[Juneezee]

Thanks for the fix! I reproduced all three problems on main (e03df0f). For example, an app using klog v2.140.0 or glog v1.0.0 fails to start, even for --help, with syntax error: expect file.go:234.

I rebased on main and simplified the change. PreParse set every flag to its DefValue (added in #2094):

cli/flag_ext.go

Lines 12 to 18 in e03df0f

	func (e *extFlag) PreParse() error {
	if e.f.DefValue != "" {
	return e.Set("", e.f.DefValue)
	}
	return nil
	}

The flag package already sets DefValue to the value's String(), so PreParse now only resets a value that differs from its default, e.g. on a second Run. A flag that rejects its own default, such as klog's -log_backtrace_at (:0), is not set on startup, so the Get() != nil check is gone. That check also panicked for a flag.Value without a Get method.

cli/flag_ext.go

Lines 13 to 23 in 57e1bcb

	func (e *extFlag) PreParse() error {
	// The flag package sets DefValue to the value's String(), so only reset
	// a value that has changed since, e.g. in a previous run. Some flags
	// don't accept their own DefValue, such as klog's -log_backtrace_at
	// (":0"), and setting it unchanged would fail.
	if e.f.DefValue != "" && e.f.Value.String() != e.f.DefValue {
	return e.Set("", e.f.DefValue)
	}
	return nil
	}

I also dropped externalValue, RunAction, IsRequired and IsMultiValueFlag. TypeName returned "" for every flag, because flag values are pointer types, which have no name. It now uses flag.UnquoteUsage, and TakesValue is true unless it is a bool flag, so help shows --log_dir string like the flag package does. IsLocal makes the flags available in subcommands, as other flags are by default.

IsSet still returns false for these flags, which can be a follow-up. I also updated the description to match.

I rebased and rewrote the whole PR

[Juneezee]

I pushed a follow-up after another look.

A Go flag is no longer added when a flag of the command already has its name. With glog, help listed -v value log level for V logs (default: 0) under a user's --verbose, -v, but the user flag always wins, so that -v could never be set:

cli/command_setup.go

Lines 82 to 88 in 1660a9d

	flag.VisitAll(func(f *flag.Flag) {
	// skip test flags, and flags whose name a flag of this command
	// already has, as that flag wins
	if !strings.HasPrefix(f.Name, ignoreFlagPrefix) && !flagNamesInUse(cmd.Flags, []string{f.Name}) {
	cmd.Flags = append(cmd.Flags, &extFlag{f: f})
	}
	})

IsDefaultVisible now returns false when DefValue is empty. Help otherwise fell back to the current value, so foo --name bob --help showed (default: bob) for a flag with no default. This replaces what I wrote above about IsDefaultVisible:

cli/flag_ext.go

Lines 62 to 64 in 1660a9d

	func (e *extFlag) IsDefaultVisible() bool {
	return e.f.DefValue != ""
	}

The new tests no longer print help, and TestExtFlagHelp covers both changes.

I also updated the commit message and the release note: a bool Go flag is now turned off with --flag=false, since --flag false sets it and passes false as an argument, as in the flag package.