From 3a9b24112ec282b766bfbcdf00caceaacd01a213 Mon Sep 17 00:00:00 2001
From: WoodenSquares <woodensquares@users.noreply.github.com>
Date: Mon, 20 Nov 2017 11:37:40 -0800
Subject: [PATCH 1/7] Initial POC, manually tested for now

---
 args.go                         | 42 +++++++++++++++++++
 commands.go                     | 68 +++++++++++++++++++++++++++----
 internal/container/container.go | 39 ++++++++++++++++++
 internal/fsm/fsm.go             | 14 +++++++
 internal/values/values.go       | 72 +++++++++++++++++++++++++++++++++
 options.go                      | 68 +++++++++++++++++++++++++++++++
 6 files changed, 296 insertions(+), 7 deletions(-)

diff --git a/args.go b/args.go
index faf3f2b..911e9f2 100644
--- a/args.go
+++ b/args.go
@@ -2,6 +2,7 @@ package cli
 
 import (
 	"flag"
+	"fmt"
 
 	"github.com/jawher/mow.cli/internal/container"
 	"github.com/jawher/mow.cli/internal/values"
@@ -27,6 +28,29 @@ func (a BoolArg) value() bool {
 	return a.Value
 }
 
+// EnumArg describes a string option
+type EnumArg struct {
+	// A space separated list of the option names *WITHOUT* the dashes, e.g. `f force` and *NOT* `-f --force`.
+	// The one letter names will then be called with a single dash (short option), the others with two (long options).
+	Name string
+	// The option description as will be shown in help messages
+	Desc string
+	// A space separated list of environment variables names to be used to initialize this option
+	EnvVar string
+	// The option's initial value
+	Value string
+	// A boolean to display or not the current value of the option in the help message
+	HideValue bool
+	// Set to true if this option was set by the user (as opposed to being set from env or not set at all)
+	SetByUser *bool
+	// Enums contains the enum values
+	Validation []EnumValidator
+}
+
+func (a EnumArg) value() string {
+	return a.Value
+}
+
 // StringArg describes a string argument
 type StringArg struct {
 	// The argument name as will be shown in help messages
@@ -143,6 +167,24 @@ func (c *Cmd) BoolArg(name string, value bool, desc string) *bool {
 	})
 }
 
+/*
+EnumArg defines an enum argument on the command c named `name`, with an initial value of `value` and a description of `desc` which will be used in help messages.
+
+The result should be stored in a variable (a pointer to a string) which will be populated when the app is run and the call arguments get parsed
+*/
+func (c *Cmd) EnumArg(name, value, desc string, validation []EnumValidator) *string {
+	if validation == nil || len(validation) == 0 {
+		panic(fmt.Sprintf("Enums require validation %s %s", name, value))
+	}
+
+	return c.Enum(EnumArg{
+		Name:       name,
+		Value:      value,
+		Desc:       desc,
+		Validation: validation,
+	})
+}
+
 /*
 StringArg defines a string argument on the command c named `name`, with an initial value of `value` and a description of `desc` which will be used in help messages.
 
diff --git a/commands.go b/commands.go
index e8c44b4..e562b48 100644
--- a/commands.go
+++ b/commands.go
@@ -56,6 +56,13 @@ type BoolParam interface {
 	value() bool
 }
 
+/*
+EnumParam represents a Enum option or argument
+*/
+type EnumParam interface {
+	value() string
+}
+
 /*
 StringParam represents a String option or argument
 */
@@ -147,6 +154,38 @@ func (c *Cmd) Bool(p BoolParam) *bool {
 	return into
 }
 
+/*
+Enum can be used to add an enum option or argument to a command.
+It accepts either a EnumOpt or a EnumArg struct.
+
+The result should be stored in a variable (a pointer to an enum) which will be populated when the app is run and the call arguments get parsed
+*/
+func (c *Cmd) Enum(p EnumParam) *string {
+	into := new(string)
+	value := values.NewEnum(into, p.value())
+
+	switch x := p.(type) {
+	case EnumOpt:
+		validation := make([]container.Validator, len(x.Validation))
+		for i, v := range x.Validation {
+			v.ToContainerValidator(&validation[i])
+		}
+
+		c.mkOpt(container.Container{Name: x.Name, Desc: x.Desc, EnvVar: x.EnvVar, HideValue: x.HideValue, Value: value, ValueSetByUser: x.SetByUser, Validation: validation})
+	case EnumArg:
+		validation := make([]container.Validator, len(x.Validation))
+		for i, v := range x.Validation {
+			v.ToContainerValidator(&validation[i])
+		}
+
+		c.mkArg(container.Container{Name: x.Name, Desc: x.Desc, EnvVar: x.EnvVar, HideValue: x.HideValue, Value: value, ValueSetByUser: x.SetByUser, Validation: validation})
+	default:
+		panic(fmt.Sprintf("Unhandled param %v", p))
+	}
+
+	return into
+}
+
 /*
 String can be used to add a string option or argument to a command.
 It accepts either a StringOpt or a StringArg struct.
@@ -358,10 +397,11 @@ func (c *Cmd) printHelp(longDesc bool) {
 
 		for _, arg := range c.args {
 			var (
-				env   = formatEnvVarsForHelp(arg.EnvVar)
-				value = formatValueForHelp(arg.HideValue, arg.Value)
+				env        = formatEnvVarsForHelp(arg.EnvVar)
+				value      = formatValueForHelp(arg.HideValue, arg.Value)
+				additional = formatExtraForHelp(arg.HideValue, arg)
 			)
-			fmt.Fprintf(w, "  %s\t%s\n", arg.Name, joinStrings(arg.Desc, env, value))
+			fmt.Fprintf(w, "  %s\t%s\n", arg.Name, joinStrings(arg.Desc, env, value, additional))
 		}
 	}
 
@@ -370,11 +410,12 @@ func (c *Cmd) printHelp(longDesc bool) {
 
 		for _, opt := range c.options {
 			var (
-				optNames = formatOptNamesForHelp(opt)
-				env      = formatEnvVarsForHelp(opt.EnvVar)
-				value    = formatValueForHelp(opt.HideValue, opt.Value)
+				optNames   = formatOptNamesForHelp(opt)
+				env        = formatEnvVarsForHelp(opt.EnvVar)
+				value      = formatValueForHelp(opt.HideValue, opt.Value)
+				additional = formatExtraForHelp(opt.HideValue, opt)
 			)
-			fmt.Fprintf(w, "  %s\t%s\n", optNames, joinStrings(opt.Desc, env, value))
+			fmt.Fprintf(w, "  %s\t%s\n", optNames, joinStrings(opt.Desc, env, value, additional))
 		}
 	}
 
@@ -419,6 +460,19 @@ func formatOptNamesForHelp(o *container.Container) string {
 	}
 }
 
+func formatExtraForHelp(hide bool, c *container.Container) string {
+	if hide {
+		return ""
+	}
+
+	extra := ""
+	for _, v := range c.Validation {
+		extra += fmt.Sprintf("\n\t  %s: %s", v.User, v.Help)
+	}
+
+	return extra
+}
+
 func formatValueForHelp(hide bool, v flag.Value) string {
 	if hide {
 		return ""
diff --git a/internal/container/container.go b/internal/container/container.go
index d5d858f..ca89565 100644
--- a/internal/container/container.go
+++ b/internal/container/container.go
@@ -2,6 +2,44 @@ package container
 
 import "flag"
 
+// ValidationType is the type of validation supported for this field
+type ValidationType int
+
+const (
+	// Enum is for enum validation, espected to set User/Value/Help
+	Enum ValidationType = iota
+
+	// TODO:
+	// IPv4Address
+	// IPv6Address
+	// IPv4Network
+	// IPv6Network
+	// IntRange
+)
+
+/*
+Validation contains information needed to validate the value provided by the
+user
+*/
+type Validator struct {
+	Type ValidationType
+
+	// TODO:
+	// IntMin int
+	// IntMax max
+	// ...
+
+	// Enums
+	// User is the value the user can pass
+	User string
+	// Value is the value assigned to the string for this user value
+	Value string
+	// Help is the help message for this value
+	Help string
+	// Callback is called if the user passes this value to the enum
+	Callback func() error
+}
+
 /*
 Container holds an option or an arg data
 */
@@ -14,4 +52,5 @@ type Container struct {
 	ValueSetFromEnv bool
 	ValueSetByUser  *bool
 	Value           flag.Value
+	Validation      []Validator
 }
diff --git a/internal/fsm/fsm.go b/internal/fsm/fsm.go
index e16208e..d2161bc 100644
--- a/internal/fsm/fsm.go
+++ b/internal/fsm/fsm.go
@@ -137,6 +137,20 @@ func fillContainers(containers map[*container.Container][]string) error {
 			multiValued.Clear()
 		}
 		for _, v := range vs {
+			// Should check for a valid value
+			if con.Validation != nil {
+				switch x := con.Value.(type) {
+				case values.EnumValued:
+					vc, err := x.Validate(v, con.Validation)
+					if err != nil {
+						return err
+					}
+					v = vc
+				default:
+					panic(fmt.Sprintf("Unhandled validator %v", x))
+				}
+			}
+
 			if err := con.Value.Set(v); err != nil {
 				return err
 			}
diff --git a/internal/values/values.go b/internal/values/values.go
index 9233c3b..ea3b5bf 100644
--- a/internal/values/values.go
+++ b/internal/values/values.go
@@ -4,6 +4,8 @@ import (
 	"flag"
 	"fmt"
 	"strconv"
+
+	"github.com/jawher/mow.cli/internal/container"
 )
 
 // BoolValued is an interface values can implement to indicate that they are a bool option, i.e. can be set without providing a value with just -f for example
@@ -13,6 +15,13 @@ type BoolValued interface {
 	IsBoolFlag() bool
 }
 
+// EnumValued is an interface ti indicate that a value can hold an enum
+type EnumValued interface {
+	flag.Value
+	// Validate makes sure the value passed is valid
+	Validate(string, []container.Validator) (string, error)
+}
+
 // MultiValued is an interface ti indicate that a value can hold multiple values
 type MultiValued interface {
 	flag.Value
@@ -69,6 +78,69 @@ func (bo *BoolValue) IsDefault() bool {
 	return !bool(*bo)
 }
 
+/******************************************************************************/
+/* ENUM                                                                        */
+/******************************************************************************/
+
+// EnumValue is a flag.Value type holding string values
+type EnumValue string
+
+var (
+	_ flag.Value    = NewEnum(new(string), "")
+	_ EnumValued    = NewEnum(new(string), "")
+	_ DefaultValued = NewEnum(new(string), "")
+)
+
+// NewEnum creates a new string value
+func NewEnum(into *string, v string) *EnumValue {
+	*into = v
+	return (*EnumValue)(into)
+}
+
+// Set sets the value from a provided string
+func (sa *EnumValue) Set(s string) error {
+	*sa = EnumValue(s)
+	return nil
+}
+
+func (sa *EnumValue) String() string {
+	return fmt.Sprintf("%#v", *sa)
+}
+
+// IsDefault return true if the string value is empty
+func (sa *EnumValue) IsDefault() bool {
+	return string(*sa) == ""
+}
+
+// Validate validates the specified enum value, returns the wanted value and
+// calls the relevant callback if defined.
+func (sa *EnumValue) Validate(v string, vv []container.Validator) (string, error) {
+	for _, x := range vv {
+		if x.User == v {
+			if x.Callback != nil {
+				err := x.Callback()
+				if err != nil {
+					return "", err
+				}
+			}
+			return x.Value, nil
+		}
+	}
+
+	// If we are here the value is invalid, let's give the user the list of
+	// valid values in our validation list.
+	help := ""
+	for i := 0; i < len(vv); i++ {
+		help += vv[i].User
+		if i == (len(vv) - 1) {
+			help += "."
+		} else {
+			help += ", "
+		}
+	}
+	return "", fmt.Errorf("Invalid value %s, valid values are %s", v, help)
+}
+
 /******************************************************************************/
 /* STRING                                                                        */
 /******************************************************************************/
diff --git a/options.go b/options.go
index da324d4..c42e809 100644
--- a/options.go
+++ b/options.go
@@ -2,6 +2,7 @@ package cli
 
 import (
 	"flag"
+	"fmt"
 	"strings"
 
 	"github.com/jawher/mow.cli/internal/container"
@@ -29,6 +30,51 @@ func (o BoolOpt) value() bool {
 	return o.Value
 }
 
+// EnumValidator allows users to validate enum values
+type EnumValidator struct {
+	// User is the value the user can pass
+	User string
+	// Value is the value assigned to the string for this user value
+	Value string
+	// Help is the help message for this value
+	Help string
+	// Callback is called if this value is set
+	Callback func() error
+}
+
+// ToContainerValidator creates a container validator (shared for all object
+// types) from an object-specific validator
+func (v *EnumValidator) ToContainerValidator(dest *container.Validator) {
+	dest.Type = container.Enum
+	dest.User = v.User
+	dest.Value = v.Value
+	dest.Help = v.Help
+	dest.Callback = v.Callback
+}
+
+// EnumOpt describes a string option
+type EnumOpt struct {
+	// A space separated list of the option names *WITHOUT* the dashes, e.g. `f force` and *NOT* `-f --force`.
+	// The one letter names will then be called with a single dash (short option), the others with two (long options).
+	Name string
+	// The option description as will be shown in help messages
+	Desc string
+	// A space separated list of environment variables names to be used to initialize this option
+	EnvVar string
+	// The option's initial value
+	Value string
+	// A boolean to display or not the current value of the option in the help message
+	HideValue bool
+	// Set to true if this option was set by the user (as opposed to being set from env or not set at all)
+	SetByUser *bool
+	// Enums contains the enum values
+	Validation []EnumValidator
+}
+
+func (o EnumOpt) value() string {
+	return o.Value
+}
+
 // StringOpt describes a string option
 type StringOpt struct {
 	// A space separated list of the option names *WITHOUT* the dashes, e.g. `f force` and *NOT* `-f --force`.
@@ -153,6 +199,28 @@ func (c *Cmd) BoolOpt(name string, value bool, desc string) *bool {
 	})
 }
 
+/*
+EnumOpt defines a enumean option on the command c named `name`, with an initial value of `value` and a description of `desc` which will be used in help messages.
+
+The name is a space separated list of the option names *WITHOUT* the dashes, e.g. `f force` and *NOT* `-f --force`.
+The one letter names will then be called with a single dash (short option), the others with two (long options).
+
+
+The result should be stored in a variable (a pointer to a enum) which will be populated when the app is run and the call arguments get parsed
+*/
+func (c *Cmd) EnumOpt(name string, value string, desc string, validation []EnumValidator) *string {
+	if validation == nil || len(validation) == 0 {
+		panic(fmt.Sprintf("Enums require validation %s %s", name, value))
+	}
+
+	return c.Enum(EnumOpt{
+		Name:       name,
+		Value:      value,
+		Desc:       desc,
+		Validation: validation,
+	})
+}
+
 /*
 StringOpt defines a string option on the command c named `name`, with an initial value of `value` and a description of `desc` which will be used in help messages.
 

From 8309e38e5d20c2fe561023fbe123a7cfde6567c2 Mon Sep 17 00:00:00 2001
From: WoodenSquares <woodensquares@users.noreply.github.com>
Date: Mon, 20 Nov 2017 14:25:01 -0800
Subject: [PATCH 2/7] Deal with environmental variables also for enums, help
 message default value is wrong

---
 cli_test.go               | 36 ++++++++++++++++++++++++++++++++++++
 commands.go               | 26 ++++++++++++++++++++++++++
 internal/values/values.go |  6 ++++++
 testdata/help-output.txt  | 18 ++++++++++++++++++
 4 files changed, 86 insertions(+)

diff --git a/cli_test.go b/cli_test.go
index b93d6ea..3df5141 100644
--- a/cli_test.go
+++ b/cli_test.go
@@ -465,6 +465,24 @@ func TestHelpMessage(t *testing.T) {
 	app.String(StringOpt{Name: "str2", Value: "a value", Desc: "String Option 2"})
 	app.String(StringOpt{Name: "u", Value: "another value", EnvVar: "STR3", Desc: "String Option 3", HideValue: true})
 
+	app.Enum(EnumOpt{Name: "e enum1", Value: "", EnvVar: "ENUM1", Desc: "Enum Option 1", Validation: []EnumValidator{
+		{User: "value1", Value: "v1", Help: "Option 1 value 1"},
+		{User: "value2", Value: "v2", Help: "Option 1 value 2"},
+		{User: "value3", Value: "v3", Help: "Option 1 value 3"},
+	}})
+
+	app.Enum(EnumOpt{Name: "enum2", Value: "a value", Desc: "Enum Option 2", Validation: []EnumValidator{
+		{User: "value1", Value: "v1", Help: "Option 2 value 1"},
+		{User: "value2", Value: "v2", Help: "Option 2 value 2"},
+		{User: "value3", Value: "v3", Help: "Option 2 value 3"},
+	}})
+
+	app.Enum(EnumOpt{Name: "f", Value: "another value", EnvVar: "ENUM3", Desc: "Enum Option 3", HideValue: true, Validation: []EnumValidator{
+		{User: "value1", Value: "v1", Help: "Option 3 value 1"},
+		{User: "value2", Value: "v2", Help: "Option 3 value 2"},
+		{User: "value3", Value: "v3", Help: "Option 3 value 3"},
+	}})
+
 	app.Int(IntOpt{Name: "i int1", Value: 0, EnvVar: "INT1 ALIAS_INT1"})
 	app.Int(IntOpt{Name: "int2", Value: 1, EnvVar: "INT2", Desc: "Int Option 2"})
 	app.Int(IntOpt{Name: "k", Value: 1, EnvVar: "INT3", Desc: "Int Option 3", HideValue: true})
@@ -486,6 +504,24 @@ func TestHelpMessage(t *testing.T) {
 	app.String(StringArg{Name: "STR2", Value: "a value", EnvVar: "STR2", Desc: "String Argument 2"})
 	app.String(StringArg{Name: "STR3", Value: "another value", EnvVar: "STR3", Desc: "String Argument 3", HideValue: true})
 
+	app.Enum(EnumArg{Name: "ENUM1", Value: "", EnvVar: "ENUM1", Desc: "Enum Argument 1", Validation: []EnumValidator{
+		{User: "value1", Value: "v1", Help: "Argument 1 value 1"},
+		{User: "value2", Value: "v2", Help: "Argument 1 value 2"},
+		{User: "value3", Value: "v3", Help: "Argument 1 value 3"},
+	}})
+
+	app.Enum(EnumArg{Name: "ENUM1", Value: "a value", Desc: "Enum Argument 2", Validation: []EnumValidator{
+		{User: "value1", Value: "v1", Help: "Argument 2 value 1"},
+		{User: "value2", Value: "v2", Help: "Argument 2 value 2"},
+		{User: "value3", Value: "v3", Help: "Argument 2 value 3"},
+	}})
+
+	app.Enum(EnumArg{Name: "ENUM3", Value: "another value", EnvVar: "ENUM3", Desc: "Enum Argument 3", HideValue: true, Validation: []EnumValidator{
+		{User: "value1", Value: "v1", Help: "Argument 3 value 1"},
+		{User: "value2", Value: "v2", Help: "Argument 3 value 2"},
+		{User: "value3", Value: "v3", Help: "Argument 3 value 3"},
+	}})
+
 	app.Int(IntArg{Name: "INT1", Value: 0, EnvVar: "INT1", Desc: "Int Argument 1"})
 	app.Int(IntArg{Name: "INT2", Value: 1, EnvVar: "INT2", Desc: "Int Argument 2"})
 	app.Int(IntArg{Name: "INT3", Value: 1, EnvVar: "INT3", Desc: "Int Argument 3", HideValue: true})
diff --git a/commands.go b/commands.go
index e562b48..20d6937 100644
--- a/commands.go
+++ b/commands.go
@@ -536,6 +536,32 @@ func (c *Cmd) parse(args []string, entry, inFlow, outFlow *flow.Step) error {
 		Exiter:  exiter,
 	}
 
+	// When an argument or option is set from the environment we haven't
+	// validated yet, we can't validate inside mkopt/mkarg because there is no
+	// way to return an error back to the user.
+	//
+	// Putting this validation here, rather than at the top of the function,
+	// means it is possible to override an invalid environmental variable with
+	// a valid commandline value
+	for _, curr := range []([]*container.Container){c.args, c.options} {
+		for _, op := range curr {
+			if op.ValueSetFromEnv && op.Validation != nil {
+				switch x := op.Value.(type) {
+				case values.EnumValued:
+					vc, err := x.Validate(x.RealString(), op.Validation)
+					if err != nil {
+						fmt.Fprintf(stdErr, "Error: %s\n", err.Error())
+						c.PrintHelp()
+						c.onError(err)
+					}
+					x.Set(vc)
+				default:
+					panic(fmt.Sprintf("Unhandled validator %v", x))
+				}
+			}
+		}
+	}
+
 	args = args[nargsLen:]
 	if len(args) == 0 {
 		if c.Action != nil {
diff --git a/internal/values/values.go b/internal/values/values.go
index ea3b5bf..4d118a8 100644
--- a/internal/values/values.go
+++ b/internal/values/values.go
@@ -20,6 +20,7 @@ type EnumValued interface {
 	flag.Value
 	// Validate makes sure the value passed is valid
 	Validate(string, []container.Validator) (string, error)
+	RealString() string
 }
 
 // MultiValued is an interface ti indicate that a value can hold multiple values
@@ -107,6 +108,11 @@ func (sa *EnumValue) String() string {
 	return fmt.Sprintf("%#v", *sa)
 }
 
+// RealString returns the underlying string set in the enum value
+func (sa *EnumValue) RealString() string {
+	return string(*sa)
+}
+
 // IsDefault return true if the string value is empty
 func (sa *EnumValue) IsDefault() bool {
 	return string(*sa) == ""
diff --git a/testdata/help-output.txt b/testdata/help-output.txt
index 1ac03c3..b5616fd 100644
--- a/testdata/help-output.txt
+++ b/testdata/help-output.txt
@@ -10,6 +10,15 @@ Arguments:
   STR1          String Argument 1 (env $STR1)
   STR2          String Argument 2 (env $STR2) (default "a value")
   STR3          String Argument 3 (env $STR3)
+  ENUM1         Enum Argument 1 (env $ENUM1) 
+                  value1: Argument 1 value 1
+                  value2: Argument 1 value 2
+                  value3: Argument 1 value 3
+  ENUM1         Enum Argument 2 (default "a value") 
+                  value1: Argument 2 value 1
+                  value2: Argument 2 value 2
+                  value3: Argument 2 value 3
+  ENUM3         Enum Argument 3 (env $ENUM3)
   INT1          Int Argument 1 (env $INT1) (default 0)
   INT2          Int Argument 2 (env $INT2) (default 1)
   INT3          Int Argument 3 (env $INT3)
@@ -27,6 +36,15 @@ Options:
   -s, --str1    String Option 1 (env $STR1)
       --str2    String Option 2 (default "a value")
   -u            String Option 3 (env $STR3)
+  -e, --enum1   Enum Option 1 (env $ENUM1) 
+                  value1: Option 1 value 1
+                  value2: Option 1 value 2
+                  value3: Option 1 value 3
+      --enum2   Enum Option 2 (default "a value") 
+                  value1: Option 2 value 1
+                  value2: Option 2 value 2
+                  value3: Option 2 value 3
+  -f            Enum Option 3 (env $ENUM3)
   -i, --int1    (env $INT1, $ALIAS_INT1) (default 0)
       --int2    Int Option 2 (env $INT2) (default 1)
   -k            Int Option 3 (env $INT3)

From 9425180f204595f4d08057f83a3b61d15d5a0eab Mon Sep 17 00:00:00 2001
From: WoodenSquares <woodensquares@users.noreply.github.com>
Date: Mon, 20 Nov 2017 15:37:15 -0800
Subject: [PATCH 3/7] Do the validation only in one place, also has the
 advantage it will validate the default values.

---
 commands.go         |  2 +-
 internal/fsm/fsm.go | 16 ++--------------
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/commands.go b/commands.go
index 20d6937..cfab932 100644
--- a/commands.go
+++ b/commands.go
@@ -545,7 +545,7 @@ func (c *Cmd) parse(args []string, entry, inFlow, outFlow *flow.Step) error {
 	// a valid commandline value
 	for _, curr := range []([]*container.Container){c.args, c.options} {
 		for _, op := range curr {
-			if op.ValueSetFromEnv && op.Validation != nil {
+			if op.Validation != nil {
 				switch x := op.Value.(type) {
 				case values.EnumValued:
 					vc, err := x.Validate(x.RealString(), op.Validation)
diff --git a/internal/fsm/fsm.go b/internal/fsm/fsm.go
index d2161bc..032ed1a 100644
--- a/internal/fsm/fsm.go
+++ b/internal/fsm/fsm.go
@@ -137,20 +137,8 @@ func fillContainers(containers map[*container.Container][]string) error {
 			multiValued.Clear()
 		}
 		for _, v := range vs {
-			// Should check for a valid value
-			if con.Validation != nil {
-				switch x := con.Value.(type) {
-				case values.EnumValued:
-					vc, err := x.Validate(v, con.Validation)
-					if err != nil {
-						return err
-					}
-					v = vc
-				default:
-					panic(fmt.Sprintf("Unhandled validator %v", x))
-				}
-			}
-
+			// Validity will be checked in commands.parse, unconditionally set
+			// the value for now.
 			if err := con.Value.Set(v); err != nil {
 				return err
 			}

From f419be9498abdbe7cd3fe4bc62b3ea971ea5b5f1 Mon Sep 17 00:00:00 2001
From: WoodenSquares <woodensquares@users.noreply.github.com>
Date: Mon, 20 Nov 2017 15:38:00 -0800
Subject: [PATCH 4/7] Deal with the issue of the default values displaying
 wrongly depending on when printvalues is called

---
 args.go                         |  4 ++++
 commands.go                     | 14 ++++++--------
 internal/container/container.go |  2 ++
 internal/values/values.go       | 18 +++++++++---------
 options.go                      |  4 ++++
 5 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/args.go b/args.go
index 911e9f2..021233a 100644
--- a/args.go
+++ b/args.go
@@ -247,6 +247,10 @@ func (c *Cmd) VarArg(name string, value flag.Value, desc string) {
 }
 
 func (c *Cmd) mkArg(arg container.Container) {
+	arg.DefaultValue = arg.Value.String()
+	if dv, ok := arg.Value.(values.DefaultValued); ok {
+		arg.DefaultDisplay = dv.IsDefault()
+	}
 	arg.ValueSetFromEnv = values.SetFromEnv(arg.Value, arg.EnvVar)
 
 	c.args = append(c.args, &arg)
diff --git a/commands.go b/commands.go
index cfab932..34b0140 100644
--- a/commands.go
+++ b/commands.go
@@ -398,7 +398,7 @@ func (c *Cmd) printHelp(longDesc bool) {
 		for _, arg := range c.args {
 			var (
 				env        = formatEnvVarsForHelp(arg.EnvVar)
-				value      = formatValueForHelp(arg.HideValue, arg.Value)
+				value      = formatValueForHelp(arg.HideValue, arg.DefaultDisplay, arg.DefaultValue)
 				additional = formatExtraForHelp(arg.HideValue, arg)
 			)
 			fmt.Fprintf(w, "  %s\t%s\n", arg.Name, joinStrings(arg.Desc, env, value, additional))
@@ -412,7 +412,7 @@ func (c *Cmd) printHelp(longDesc bool) {
 			var (
 				optNames   = formatOptNamesForHelp(opt)
 				env        = formatEnvVarsForHelp(opt.EnvVar)
-				value      = formatValueForHelp(opt.HideValue, opt.Value)
+				value      = formatValueForHelp(opt.HideValue, opt.DefaultDisplay, opt.DefaultValue)
 				additional = formatExtraForHelp(opt.HideValue, opt)
 			)
 			fmt.Fprintf(w, "  %s\t%s\n", optNames, joinStrings(opt.Desc, env, value, additional))
@@ -473,18 +473,16 @@ func formatExtraForHelp(hide bool, c *container.Container) string {
 	return extra
 }
 
-func formatValueForHelp(hide bool, v flag.Value) string {
+func formatValueForHelp(hide bool, dontDisplay bool, vp string) string {
 	if hide {
 		return ""
 	}
 
-	if dv, ok := v.(values.DefaultValued); ok {
-		if dv.IsDefault() {
-			return ""
-		}
+	if dontDisplay {
+		return ""
 	}
 
-	return fmt.Sprintf("(default %s)", v.String())
+	return fmt.Sprintf("(default %s)", vp)
 }
 
 func formatEnvVarsForHelp(envVars string) string {
diff --git a/internal/container/container.go b/internal/container/container.go
index ca89565..c6e5bc8 100644
--- a/internal/container/container.go
+++ b/internal/container/container.go
@@ -52,5 +52,7 @@ type Container struct {
 	ValueSetFromEnv bool
 	ValueSetByUser  *bool
 	Value           flag.Value
+	DefaultValue    string
+	DefaultDisplay  bool
 	Validation      []Validator
 }
diff --git a/internal/values/values.go b/internal/values/values.go
index 4d118a8..c660066 100644
--- a/internal/values/values.go
+++ b/internal/values/values.go
@@ -99,28 +99,28 @@ func NewEnum(into *string, v string) *EnumValue {
 }
 
 // Set sets the value from a provided string
-func (sa *EnumValue) Set(s string) error {
-	*sa = EnumValue(s)
+func (ea *EnumValue) Set(s string) error {
+	*ea = EnumValue(s)
 	return nil
 }
 
-func (sa *EnumValue) String() string {
-	return fmt.Sprintf("%#v", *sa)
+func (ea *EnumValue) String() string {
+	return fmt.Sprintf("%#v", *ea)
 }
 
 // RealString returns the underlying string set in the enum value
-func (sa *EnumValue) RealString() string {
-	return string(*sa)
+func (ea *EnumValue) RealString() string {
+	return string(*ea)
 }
 
 // IsDefault return true if the string value is empty
-func (sa *EnumValue) IsDefault() bool {
-	return string(*sa) == ""
+func (ea *EnumValue) IsDefault() bool {
+	return string(*ea) == ""
 }
 
 // Validate validates the specified enum value, returns the wanted value and
 // calls the relevant callback if defined.
-func (sa *EnumValue) Validate(v string, vv []container.Validator) (string, error) {
+func (ea *EnumValue) Validate(v string, vv []container.Validator) (string, error) {
 	for _, x := range vv {
 		if x.User == v {
 			if x.Callback != nil {
diff --git a/options.go b/options.go
index c42e809..5222c3d 100644
--- a/options.go
+++ b/options.go
@@ -315,6 +315,10 @@ func mkOptStrs(optName string) []string {
 }
 
 func (c *Cmd) mkOpt(opt container.Container) {
+	opt.DefaultValue = opt.Value.String()
+	if dv, ok := opt.Value.(values.DefaultValued); ok {
+		opt.DefaultDisplay = dv.IsDefault()
+	}
 	opt.ValueSetFromEnv = values.SetFromEnv(opt.Value, opt.EnvVar)
 
 	opt.Names = mkOptStrs(opt.Name)

From 3e62d1e9e63305dfcc438781ce724ef0909bfe10 Mon Sep 17 00:00:00 2001
From: WoodenSquares <woodensquares@users.noreply.github.com>
Date: Tue, 21 Nov 2017 13:15:43 -0800
Subject: [PATCH 5/7] Added a test for the default shadowing issue, plus update
 the golden file

---
 cli_test.go              | 39 +++++++++++++++++++++++++++++++++++++--
 testdata/help-output.txt |  4 ++--
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/cli_test.go b/cli_test.go
index 3df5141..e65c554 100644
--- a/cli_test.go
+++ b/cli_test.go
@@ -3,6 +3,7 @@ package cli
 import (
 	"bytes"
 	"flag"
+	"strings"
 
 	"github.com/stretchr/testify/require"
 
@@ -454,7 +455,7 @@ func TestHelpMessage(t *testing.T) {
 	defer exitShouldBeCalledWith(t, 0, &exitCalled)()
 
 	app := App("app", "App Desc")
-	app.Spec = "[-bdsuikqs] BOOL1 [STR1] INT3..."
+	app.Spec = "[-bdesuikqs] BOOL1 [STR1] INT3..."
 
 	// Options
 	app.Bool(BoolOpt{Name: "b bool1 u uuu", Value: false, EnvVar: "BOOL1", Desc: "Bool Option 1"})
@@ -510,7 +511,7 @@ func TestHelpMessage(t *testing.T) {
 		{User: "value3", Value: "v3", Help: "Argument 1 value 3"},
 	}})
 
-	app.Enum(EnumArg{Name: "ENUM1", Value: "a value", Desc: "Enum Argument 2", Validation: []EnumValidator{
+	app.Enum(EnumArg{Name: "ENUM2", Value: "a value", Desc: "Enum Argument 2", Validation: []EnumValidator{
 		{User: "value1", Value: "v1", Help: "Argument 2 value 1"},
 		{User: "value2", Value: "v2", Help: "Argument 2 value 2"},
 		{User: "value3", Value: "v3", Help: "Argument 2 value 3"},
@@ -1817,6 +1818,40 @@ func TestBeforeAndAfterFlowOrderWhenMultipleAftersPanic(t *testing.T) {
 	require.Equal(t, 7, counter)
 }
 
+// In some cases depending on where the validation failure occurs, the
+// assignment to variables could have been done before the help is printed,
+// making the defaults wrong (would print the assigned value instead of the
+// actual default value).
+//
+// TODO: Also note in one case it would print 'Error: Incorrect usage' while
+// in the other it wouldn't, this should probably be fixed, the testcase below
+// ignores that by looking for the common portions.
+func TestDefaultValueShadowing(t *testing.T) {
+	var out, err string
+	defer captureAndRestoreOutput(&out, &err)()
+
+	app := App("test", "")
+	app.Spec = "[-t] ARG"
+	app.String(StringOpt{Name: "t tcomm", Value: "tdefault", Desc: "somedesc"})
+	app.String(StringArg{Name: "ARG", Value: "argdefault", Desc: "somedesc"})
+	app.Command("command1", "command1 description", func(cmd *Cmd) {})
+
+	app.ErrorHandling = flag.ContinueOnError
+
+	// First run, arg ok, bad command
+	app.Run([]string{"test", "-t", "tvalue", "somearg", "badcommand"})
+	startfirst := strings.Index(err, "Usage: test [-t]")
+	errfirst := err[startfirst:]
+
+	// Second run, missing arg, still bad command
+	app.Run([]string{"test", "-t", "tvalue", "badcommand"})
+	errsecond := err[startfirst+len(errfirst):]
+	errsecond = errsecond[strings.Index(errsecond, "Usage: test [-t]"):]
+
+	require.Equal(t, errfirst, errsecond)
+
+}
+
 func exitShouldBeCalledWith(t *testing.T, wantedExitCode int, called *bool) func() {
 	oldExiter := exiter
 	exiter = func(code int) {
diff --git a/testdata/help-output.txt b/testdata/help-output.txt
index b5616fd..43f258c 100644
--- a/testdata/help-output.txt
+++ b/testdata/help-output.txt
@@ -1,5 +1,5 @@
 
-Usage: app [-bdsuikqs] BOOL1 [STR1] INT3... COMMAND [arg...]
+Usage: app [-bdesuikqs] BOOL1 [STR1] INT3... COMMAND [arg...]
 
 App Desc
                 
@@ -14,7 +14,7 @@ Arguments:
                   value1: Argument 1 value 1
                   value2: Argument 1 value 2
                   value3: Argument 1 value 3
-  ENUM1         Enum Argument 2 (default "a value") 
+  ENUM2         Enum Argument 2 (default "a value") 
                   value1: Argument 2 value 1
                   value2: Argument 2 value 2
                   value3: Argument 2 value 3

From 957de008502c9a853c15444c46ed85dd4c913ebb Mon Sep 17 00:00:00 2001
From: WoodenSquares <woodensquares@users.noreply.github.com>
Date: Tue, 21 Nov 2017 13:32:23 -0800
Subject: [PATCH 6/7] Added some basic tests

---
 cli_test.go | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 168 insertions(+)

diff --git a/cli_test.go b/cli_test.go
index e65c554..6db5b64 100644
--- a/cli_test.go
+++ b/cli_test.go
@@ -774,6 +774,45 @@ func TestOptSetByUser(t *testing.T) {
 			expected: true,
 		},
 
+		// Enum
+		{
+			desc: "Enum Opt, not set by user, default value",
+			config: func(c *Cli, s *bool) {
+				c.Enum(EnumOpt{Name: "f", Value: "v1", SetByUser: s,
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: false,
+		},
+		{
+			desc: "Enum Opt, not set by user, env value",
+			config: func(c *Cli, s *bool) {
+				os.Setenv("MOW_VALUE", "v2")
+				c.Enum(EnumOpt{Name: "f", EnvVar: "MOW_VALUE", SetByUser: s,
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: false,
+		},
+		{
+			desc: "Enum Opt, set by user",
+			config: func(c *Cli, s *bool) {
+				c.Enum(EnumOpt{Name: "f", Value: "a", SetByUser: s,
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test", "-f=v2"},
+			expected: true,
+		},
+
 		// Bool
 		{
 			desc: "Bool Opt, not set by user, default value",
@@ -941,6 +980,48 @@ func TestArgSetByUser(t *testing.T) {
 			expected: true,
 		},
 
+		// Enum
+		{
+			desc: "Enum Arg, not set by user, default value",
+			config: func(c *Cli, s *bool) {
+				c.Spec = "[ARG]"
+				c.Enum(EnumArg{Name: "ARG", Value: "v1", SetByUser: s,
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: false,
+		},
+		{
+			desc: "Enum Arg, not set by user, env value",
+			config: func(c *Cli, s *bool) {
+				c.Spec = "[ARG]"
+				os.Setenv("MOW_VALUE", "v2")
+				c.Enum(EnumArg{Name: "ARG", EnvVar: "MOW_VALUE", SetByUser: s,
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: false,
+		},
+		{
+			desc: "Enum Arg, set by user",
+			config: func(c *Cli, s *bool) {
+				c.Spec = "[ARG]"
+				c.Enum(EnumArg{Name: "ARG", Value: "a", SetByUser: s,
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test", "v2"},
+			expected: true,
+		},
+
 		// Bool
 		{
 			desc: "Bool Arg, not set by user, default value",
@@ -1120,6 +1201,48 @@ func TestOptSetByEnv(t *testing.T) {
 			expected: "user",
 		},
 
+		// Enum
+		{
+			desc: "Enum Opt, empty env var",
+			config: func(c *Cli) interface{} {
+				os.Setenv("MOW_VALUE", "")
+				return c.Enum(EnumOpt{Name: "f", Value: "v1", EnvVar: "MOW_VALUE",
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: "1",
+		},
+		{
+			desc: "Enum Opt, env set, not set by user",
+			config: func(c *Cli) interface{} {
+				os.Setenv("MOW_VALUE", "v2")
+				return c.Enum(EnumOpt{Name: "f", Value: "v1", EnvVar: "MOW_VALUE",
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: "2",
+		},
+		{
+			desc: "Enum Opt, env set, set by user",
+			config: func(c *Cli) interface{} {
+				os.Setenv("MOW_VALUE", "v2")
+				return c.Enum(EnumOpt{Name: "f", Value: "v1", EnvVar: "MOW_VALUE",
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+						{User: "v3", Value: "3", Help: "v3"},
+					}})
+			},
+			args:     []string{"test", "-f=v3"},
+			expected: "3",
+		},
+
 		// Bool
 		{
 			desc: "Bool Opt, empty env var",
@@ -1371,6 +1494,51 @@ func TestArgSetByEnv(t *testing.T) {
 			expected: "user",
 		},
 
+		// Enum
+		{
+			desc: "Enum Arg, empty env var",
+			config: func(c *Cli) interface{} {
+				c.Spec = "[ARG]"
+				os.Setenv("MOW_VALUE", "")
+				return c.Enum(EnumArg{Name: "ARG", Value: "v1", EnvVar: "MOW_VALUE",
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: "1",
+		},
+		{
+			desc: "Enum Arg, env set, not set by user",
+			config: func(c *Cli) interface{} {
+				c.Spec = "[ARG]"
+				os.Setenv("MOW_VALUE", "v2")
+				return c.Enum(EnumArg{Name: "ARG", Value: "default", EnvVar: "MOW_VALUE",
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+					}})
+			},
+			args:     []string{"test"},
+			expected: "2",
+		},
+		{
+			desc: "Enum Arg, env set, set by user",
+			config: func(c *Cli) interface{} {
+				c.Spec = "[ARG]"
+				os.Setenv("MOW_VALUE", "v2")
+				return c.Enum(EnumArg{Name: "ARG", Value: "v1", EnvVar: "MOW_VALUE",
+					Validation: []EnumValidator{
+						{User: "v1", Value: "1", Help: "v1"},
+						{User: "v2", Value: "2", Help: "v2"},
+						{User: "v3", Value: "3", Help: "v3"},
+					}})
+			},
+			args:     []string{"test", "v3"},
+			expected: "3",
+		},
+
 		// Bool
 		{
 			desc: "Bool Arg, empty env var",

From 7ab417e5f598d5119adee4f40f531e04dcf4faec Mon Sep 17 00:00:00 2001
From: WoodenSquares <woodensquares@users.noreply.github.com>
Date: Tue, 21 Nov 2017 13:48:05 -0800
Subject: [PATCH 7/7] Typo in the comment

---
 internal/container/container.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internal/container/container.go b/internal/container/container.go
index c6e5bc8..68e44b8 100644
--- a/internal/container/container.go
+++ b/internal/container/container.go
@@ -18,7 +18,7 @@ const (
 )
 
 /*
-Validation contains information needed to validate the value provided by the
+Validator contains information needed to validate the value provided by the
 user
 */
 type Validator struct {