Adding custom validation for any type of flag's value

[ErfanMomeniii]

fixes https://github.com/spf13/pflag/issues/236, fixeshttps://github.com/spf13/pflag/issues/293, fixes https://github.com/spf13/pflag/issues/299 Hi, After merging this pull request we can add custom validation for any type of flag.

It is optional and for this operation, you should define a function and pass it as the last parameter to the flag definition function.

var flagvar int
func init() {
	validation := func(value int)error{
		if value <= 4 {
			return errors.New("int value should be greater than 4")
        }
    }
	
    flag.IntVar(&flagvar, "flagname", 1234, "help message for flagname", validation)

In the above example if you pass an integer value greater than 4, it returned an error and the value of the flag doesn't set.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: Erfan Momeni
:x: ErfanMomeniii

---

Erfan Momeni seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[memreflect]

This would break existing code that uses pflag, which means many imported packages like Cobra and Viper would break.

Is there a reason why you favor this approach over the addition of a new Var implementation?

[memreflect]

I see now that it is indeed optional, so i apologize for my misteak.

we should have this in all Var, VarP, VarPF implementations

Except it currently doesn't work for Var, VarP, and VarPF implementations:

package main

import (
	"errors"

	flag "github.com/erfanmomeniii/pflag"
)

type Foo struct{}

func validateFoo(v interface{}) error {
	return errors.New("you will never see this error")
}
func (Foo) Set(string) error {
	return nil
}
func (Foo) String() string {
	return "Foo!"
}
func (Foo) Type() string {
	return "Foo"
}

func main() {
	var foo Foo
	flag.Var(foo, "foo", "usage for --foo", validateFoo)
	flag.CommandLine.Parse([]string{"--foo=FOO"})
}

Also, it's important to note that Set(string) error already performs validation on the base value (e.g. strconv.ParseInt for int, int8, int16, int32, int64), so this really should only be used to augment that. Your current implementation considers validation an essential step before setting the value, but i'm suggesting that validation should occur after an attempt to set the value. You might instead do something like this when setting the value:

func setValue(value flag.Value, newVal string, validation func(newValue flag.Value) error) (err error) {
	oldVal := value.String()
	defer func() {
		if r := recover(); r != nil {
			value.Set(oldVal)
			err = fmt.Errorf("%v", r)
		}
	}()
	if err = value.Set(newVal); err != nil {
		value.Set(oldVal)
	} else if err = validation(value); err != nil {
		value.Set(oldVal)
	}
	return
}

Notice the validation function accepts a flag.Value. Obviously this is not ideal for ints, bools, etc. that have hidden Value implementations, so you would need to wrap them. For example, here's a revision of (*FlagSet).BoolVarP:

func (f *FlagSet) BoolVarP(p *bool, name, shorthand string, value bool, usage string, validation ...func(value bool) error) {
	var flag *Flag
	if len(validation) == 0 {
		flag = f.VarPF(newBoolValue(value, p), name, shorthand, usage)
	} else {
		validationFunc := func(v Value) error {
			return validation[0](*v.(*boolValue))
		}
		flag = f.VarPF(newBoolValue(value, p), name, shorthand, usage, validationFunc)
	}
	flag.NoOptDefVal = "true"
}

Otherwise, Var, VarP, and VarPF could all accept an optional validation function of type func(Value) error. You might also store and use multiple validation functions since you can accept more than one validation function with ...func(T) error, but i think it's more important to first get validation working for all types and to consider how it interacts with the existing Set(string) error method for all Value implementations.

[memreflect]

I also forgot to mention that Var, VarP and (*FlagSet).Var, (*FlagSet).VarP, (*FlagSet).VarPF currently accept different validation types, which could be fixed easily by instead using func(Value) error after an attempt to set the flag's value.

If you insist on validating before an attempt to set the value, it's worth considering the addition of an optional Parse(strValue string) (value interface{}, err error) method to Value (or expose the stringConv, intConv, etc. functions as a Convert method since the arguments and return values already match), similar to how IsBoolFlag() bool is not a formal part of the Value interface in Go's standard flag package. This way, validation could be simplified to the type func(value interface{}) error after verifying that the error returned by the Parse method is nil.

You might also consider reverting the changes to Bool, BoolP, Int, etc. and instead just keep the additional Validation field in the Flag struct that is used to validate the value. This is how NoOptDefVal works, and there's no reason to accept more than one validation function just for the sake of a single optional validation function.