urfave / cli

A simple, fast, and fun package for building command line apps in Go
https://cli.urfave.org
MIT License
21.89k stars 1.69k forks source link

Bug in v3: Count is off by 1 #1930

Open Skeeve opened 3 days ago

Skeeve commented 3 days ago

Checklist

What problem did you observe?

When using cmd.Count("verbose") on my bool flag, it's value is always one too high

Test Program

package main

import (
    "context"
    "fmt"
    "os"

    "github.com/urfave/cli/v3"
)

func main() {
    cmd := &cli.Command{
        Name:                   "test",
        Usage:                  "Tool for testing",
        HideHelp:               true,
        UseShortOptionHandling: true,
        Flags: []cli.Flag{
            &cli.BoolFlag{
                Name:    "verbose",
                Aliases: []string{"v"},
                Usage:   "Be verbose",
            },
        },
        Action: tester,
    }

    if err := cmd.Run(context.Background(), []string{"prog", "-v"}); err != nil {
        fmt.Fprintln(os.Stderr, err)
        os.Exit(1)
    }
}

func tester(_ context.Context, cmd *cli.Command) (err error) {
    fmt.Printf(`verbosecount: %d`, cmd.Count("verbose"))
    return
}

Also on playground: https://play.golang.com/p/om6L1TD_6s5

dearchap commented 2 days ago

@Skeeve This looks very much like #1737 which was fixed in v2 but not ported to v3. Think you can make a PR ?

Skeeve commented 2 days ago

Think you can make a PR ?

I doubt it :(

I THINK the issue might be due to these two lines:

https://github.com/urfave/cli/blob/c4cd0a51cc113a90eca318cc8f7d328f05a5f2b7/flag_impl.go#L169

and

https://github.com/urfave/cli/blob/c4cd0a51cc113a90eca318cc8f7d328f05a5f2b7/flag_bool.go#L66

But I'm not so deep in understanding the source that I can tell for sure.

I'm also wondering: It seems, ALL flags are counted. Why is Count() only available for BoolfFlag? Sure, it's the only Flag wit Countable set, but what's the purpose hiding this info from the user, when it's already available?

dearchap commented 1 day ago

No one has requested count for non-bool flags. Not sure when I can get around to a fix though.

Skeeve commented 1 day ago

No one has requested count for non-bool flags. Not sure when I can get around to a fix though.

That "fix" (if it is a fix) should be easy. Just get rid of "if Countable" in the Count function.

dearchap commented 19 hours ago

@Skeeve Would you like to submit a PR with the change ? I'll get glad to review it.

Skeeve commented 17 hours ago

I would like to, but I can't. I have the slight feeling there is more wrong to it, than I first thought.

Here are my observations.

  1. Count() returns the flag's countmember
  2. Flag has baseBoolConfig which has a member Count
  3. This Count is never incremented nor returned. At least I was unable to find it and I assume this should have been done.

I created a test command for counting bools.

I also thought about counting all flags. I think it only makes sense for the …SliceFlags as all the others usually are not used more than once.