Cli: Adding Flag Validators

[johnwyles]

I think it could be invaluable to have three features that instantly would be of use in a clean and succinct way when further examining my own use cases for this library around validation:

• Have the ability to access the original flags default value (perhaps this is already possible?)
• Add flag specific built-in validation attributes to flags (e.g. Min/Max values, Regex match, etc.)
• Add a custom validator attribute which can pass/fail a flag value in an anonymous or named function

I illustrate this and provide comments in the pseudocode in this example:

package main

import (
    "fmt"
    "log"
    "os"

    "github.com/urfave/cli"
)

func main() {
    app := cli.NewApp()
    app.Action = rootAction
    app.Flags = []cli.Flag{
        cli.IntFlag{
            Name:  "timeout, t",
            // We would like to access this value later as the default value
            Value: 60,
            // These are "built-in" validators for the "int" type
            MinValue: 0,
            MaxValue: 600,
            // This is a custom anonymous or named function boolean validator
            CustomValidator: timeoutValidator,
        },
    }
    app.Name = "test-flag-value"

    if err := app.Run(os.Args); err != nil {
        log.Fatalf("An error occurred trying to run the application: %s", err)
    }
}

func rootAction(c *cli.Context) error {
    fmt.Printf("Flag TIMEOUT Value: %#v\n", c.GlobalInt("timeout"))
    fmt.Printf("Flag TIMEOUT Default Value: %#v\n", c.GlobalIntDefaultValue("timeout"))
    fmt.Printf("Flag TIMEOUT Default Min Value: %#v", c.GlobalIntDefaultMinValue("timeout"))
    fmt.Printf("Flag TIMEOUT Default Max Value: %#v", c.GlobalIntDefaultMaxValue("timeout"))

    if cli.GlobalInt("timeout") < 15 {
        // Access the original default value
        fmt.Printf("The supplied value is less than 15, resetting to: %s", c.GlobalIntDefaultValue("timeout"))
        cli.GlobalSet("timeout", c.GlobalIntDefaultValue("timeout"))
    }

    return nil
}

func timeoutValidator(c *cli.Context) bool {
    if c.GlobalInt("timeout") < 30 {
        return false
    }

    return true
}

Currently, as it stands, once the user supplies a value to the flag I have no way of knowing what the default value was associated with that flag as it seems to be overridden (I could be mistaken).

Secondly it would be a nice-to-have to have not only some custom validators for each type of flag (int: min, max, non-zero, etc., string: regex, non-nil, etc. float: percision, etc.).

Lastly having the ability supply a custom callback validator could allow users to have a catch-all for validation of the flag values.

I started to take a look into doing this but, again if I am not mistaken, it looked like the flagSet was using the underlying go built-in flag library which may prohibit accessing the default value supplied without needing these features there first. If not keeping a copy of them before modification would be a workaround if that is possible.

I would be glad to help and add more to this discussion!

[michaeljs1990]

These all sound like awesome features to add into this library. I think one of the biggest challenges facing adding in these is the way the flag structs are currently generated for this library. Currently a python script pops out the implementations for all of these and adding custom attributes like Min and Max that only apply to Ints/Floats and would make that interface significantly more complicated.

The custom validator function however seems like something that could reasonably be added without causing significant rework of how flags are currently handled. It also gives end users the most functionality.

[asahasrabuddhe]

@lynncyrin @AudriusButkevicius this sounds like a cool feature to implement

[lynncyrin]

I'd love to see someone work on the feature, yeah!

[asahasrabuddhe]

dibs!

[asahasrabuddhe]

This is how I think the validation should work:

1. Int, Int64, IntSlice, Int64Slice, Float64, Uint, Uint64 - min, max, custom validation function
2. String, StringSlice - regexp, custom validation function
3. GenericFlag - custom validation function
4. Bool / BoolT, Duration - none

I am also thinking of adding Uint Slice and Uint64 Slice flags to the current mix of already supported flags.

If we find a more popular use case, we can have an out of the box validator for it shipped with the library.

EDIT: Add Duration Flag

[AudriusButkevicius]

I've made comments in the PR which I think this is a bad feature that should be avoided. People can do validation in their command, as flag validation might be contextual to the command.

[lynncyrin]

It looks like we've got 2 👍's (me + @asahasrabuddhe) and 1 👎 (@AudriusButkevicius) from the maintainers here. There's no clear pattern for resolving the discussion here, so I'd like to propose one:

@johnwyles / @michaeljs1990 please post here with any more information you have about this feature / why you want it. I'll leave this open for a few weeks, and close it if it doesn't look like there's any consensus.

[johnwyles]

I still throw in my vote for this feature obviously. I think validation would be a great trick up urface/cli.v2 sleeves that spf13/viper I do not think has and which would take this package to the next level for others.

[lynncyrin]

I'm in favor of this feature conceptually.

That said, as a practical matter I think adding it to the package now would produce a ton of maintainer burden. I think we (eg. the maintainers) are very behind the curve as far as maintainence is concerned, so anything that adds maintainer burden should come attached with us getting more maintainers 🙂

I would be willing to reconsider this opinion if this feature got a ton of support, similarly to the 50 👍s for required flags https://github.com/urfave/cli/issues/85.

Given all that, I'm currently a 👎 on adding this, but very much want to keep this space open for conversation.

[johnwyles]

@lynncyrin on your 👎 I would like to hear why such a simple PR couldn't be tossed together - again when I have time which always seems to be fleeting - it does seem to me this is a logical addition to this great library would should be incorporated. Would love to hear your opinion since a rejiggering of https://github.com/urfave/cli/blob/v2/flag.go or such (maybe altsrc?) could be a nice little side addition to add further value

[lynncyrin]

I would like to hear why such a simple PR couldn't be tossed together ...

you answered your own question 🙂

time [is always] fleeting

and my focus is https://github.com/urfave/cli/pull/892/ right now.

After that is done, I'll remove my 👎

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[stale[bot]]

Closing this as it has become stale.