Add support for comma separated IntSliceFlag and Int64SliceFlag

[zemzale]

What type of PR is this?

• bug
• cleanup
• documentation
• feature

What this PR does / why we need it:

IntSliceFlag can't parse comma separated like this -flag 1,2,3.
This PR adds support for that by :

• Checking for comma in IntSliceFlag.Set and Int64SliceFlag.Set
• If comma exists split the value string and add each number to the underlying slice

Which issue(s) this PR fixes:

Fixes #1097

Special notes for your reviewer:

My only knowledge of this project is from using it.
I came up with a pretty naive solution to this problem, so I would be happy to know if the change is implemented in the right place.

Testing

Added unit tests :

• TestParseMultiIntSliceCommaSeperated
• TestParseMultiInt64SliceCommaSeperated

These tests wouldn't fail if the error of Run isn't checked.
All the other parsing tests do the checking in Action but these tests won't even get there if the value is not valid.

Release Notes

Add support for comma separated `IntSliceFlag` and `Int64SliceFlag`

[coilysiren]

At a high level, I wondering if this is a breaking change for people who do --flag 100,000,000 and expect [100000000] not [100,000,000]?

[rliebz]

@lynncyrin The code currently errors on that input. Consider the following program:

package main

import (
	"fmt"
	"log"
	"os"

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

func main() {
	app := cli.NewApp()
	app.Flags = []cli.Flag{
		&cli.IntSliceFlag{
			Name: "val",
		},
	}

	app.Action = func(c *cli.Context) error {
		fmt.Println(c.IntSlice("val"))
		return nil
	}

	if err := app.Run(os.Args); err != nil {
		log.Fatal(err)
	}
}

With go run main.go -val 5000 we get [5000]. If we pass 5,000, we get:

Incorrect Usage. invalid value "5,000" for flag -val: strconv.ParseInt: parsing "5,000": invalid syntax

That means we can add support for this feature without breaking backward compatibility.

I think that's a good call out that it allows potentially ambiguous inputs, but I'm okay with it in this case. We don't support that style of input in any other context (e.g., IntFlag), and it's consistent with how we handle slices of strings.

[coilysiren]

Thanks for the example @rliebz ✨

[rliebz]

Code looks great, thanks for putting tests together. One small nitpick in two places, and question:

Should we be fixing float64 slices as well? From my initial look at the code, I think that would just be an identical change in flag_float64_slice.go, so that probably belongs in this PR.

[zemzale]

Should we be fixing float64 slices as well? From my initial look at the code, I think that would just be an identical change in flag_float64_slice.go, so that probably belongs in this PR.

Yeah, I can do that.
Also searching through the tests, I can see that there are no tests TestParseMultiFloat64Slice like there are for IntSlice so I am gonna add those too while I am at it.

[saschagrunert]

I think the string slice flag does not support such behavior, right? That's why I had to add a workaround in a urfave/cli consuming project. I know that comma separation could be intended even for string slice flags, but maybe we would like to add an option for such cases as well.

[zemzale]

You are right that it doesn't support comma separated string slices from command line.
If you pass string slice flag -s foo,bar the value will be ["foo,bar"] not ["foo", "bar"].

[saschagrunert]

You are right that it doesn't support comma separated string slices from command line.
If you pass string slice flag -s foo,bar the value will be ["foo,bar"] not ["foo", "bar"].

We could add an option which enables that kind of parsing 🤔

[zemzale]

It could be done but I have no idea how I should implement that since this is the first time me touching this codebase aside from using it in my own applications 😅

[rliebz]

Sorry, I was under the impression that we were adding this to make it consistent with the behavior for strings.

The original issue (#1097) specifically cites:

This works for StringSliceFlag.

If that's not the behavior for string slices, then is #1097 really a bug, or just a misunderstanding of how the library was designed to work?

If we don't yet support this behavior for any sort of slice flag, I would actually caution against adding it at all. We can't actually make string slices behave this way at this point without breaking backward compatibility. Although it's not trivial, it is absolutely possible to get this behavior for very customized CLIs by implementing the cli.Flag interface manually.

So my recommendation here unfortunately would be not to add in this behavior right now.

If the change is desired, the process should be to open a feature request rather than a bug ticket, weigh whether we actually want this behavior against the cost of making a breaking change for StringSliceFlag, and then should we accept it, make sure the PR that implements it covers all four of the flag slice types together.

[saschagrunert]

I agree with @rliebz and I guess we should not automate any splitting. I think it would make sense in general to be added as optional feature.

[zemzale]

If that's not the behavior for string slices, then is #1097 really a bug, or just a misunderstanding of how the library was designed to work?

It must be a misunderstanding. Following programm :

package main

import (
        "fmt"
        "log"
        "os"

        "github.com/micro/cli"
)

func main() {
        app := cli.NewApp()
        app.Flags = []cli.Flag{
                &cli.StringSliceFlag{
                        Name: "val",
                },
        }

        app.Action = func(c *cli.Context) error {
                fmt.Println(c.StringSlice("val"))
                return nil
        }

        if err := app.Run(os.Args); err != nil {
                log.Fatal(err)
        }
}

When run go run main.go -val foo,bar would return [foo,bar] instead of [foo bar]
It doesn't fail with a parse error like in the issue, it just reads the the given string as one value.

So this really is not a bug, but I would be glad to open up this as a feature request and implement it.

[coilysiren]

Here's another example that builds off the one above

package main

import (
	"fmt"
	"log"
	"os"

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

func main() {
	app := cli.NewApp()
	app.Flags = []cli.Flag{
		&cli.StringSliceFlag{
			Name: "val",
		},
	}

	app.Action = func(c *cli.Context) error {
		val := c.StringSlice("val")
		fmt.Println(val)
		fmt.Println(len(val))
		return nil
	}

	if err := app.Run(os.Args); err != nil {
		log.Fatal(err)
	}
}

$ go run main.go -val cat,hat
[cat,hat]
1

So yep, looks like StringSliceFlag doesn't split on ,

[zemzale]

Then I am closing this PR and gonna create a feature request for this since there are people that are interested in this kinda functionality