Merge pull request #502 from urfave/fix-is-set-for-env

Fix context.(Global)IsSet to respect environment variables

context.go

@@ -3,6 +3,8 @@ package cli
 import (
 	"errors"
 	"flag"
+	"os"
+	"reflect"
 	"strings"
 )
 
@@ -11,12 +13,11 @@ import (
 // can be used to retrieve context-specific Args and
 // parsed command-line options.
 type Context struct {
-	App            *App
+	App           *App
-	Command        Command
+	Command       Command
-	flagSet        *flag.FlagSet
+	flagSet       *flag.FlagSet
-	setFlags       map[string]bool
+	setFlags      map[string]bool
-	globalSetFlags map[string]bool
+	parentContext *Context
-	parentContext  *Context
 }
 
 // NewContext creates a new context. For use in when invoking an App or Command action.
@@ -43,28 +44,70 @@ func (c *Context) GlobalSet(name, value string) error {
 func (c *Context) IsSet(name string) bool {
 	if c.setFlags == nil {
 		c.setFlags = make(map[string]bool)
+
 		c.flagSet.Visit(func(f *flag.Flag) {
 			c.setFlags[f.Name] = true
 		})
+
+		c.flagSet.VisitAll(func(f *flag.Flag) {
+			if _, ok := c.setFlags[f.Name]; ok {
+				return
+			}
+			c.setFlags[f.Name] = false
+		})
+
+		// XXX hack to support IsSet for flags with EnvVar
+		//
+		// There isn't an easy way to do this with the current implementation since
+		// whether a flag was set via an environment variable is very difficult to
+		// determine here. Instead, we intend to introduce a backwards incompatible
+		// change in version 2 to add `IsSet` to the Flag interface to push the
+		// responsibility closer to where the information required to determine
+		// whether a flag is set by non-standard means such as environment
+		// variables is avaliable.
+		//
+		// See https://github.com/urfave/cli/issues/294 for additional discussion
+		flags := c.Command.Flags
+		if c.Command.Name == "" { // cannot == Command{} since it contains slice types
+			if c.App != nil {
+				flags = c.App.Flags
+			}
+		}
+		for _, f := range flags {
+			eachName(f.GetName(), func(name string) {
+				if isSet, ok := c.setFlags[name]; isSet || !ok {
+					return
+				}
+
+				envVars := reflect.ValueOf(f).FieldByName("EnvVar").String()
+
+				eachName(envVars, func(envVar string) {
+					envVar = strings.TrimSpace(envVar)
+					if envVal := os.Getenv(envVar); envVal != "" {
+						c.setFlags[name] = true
+						return
+					}
+				})
+			})
+		}
 	}
-	return c.setFlags[name] == true
+
+	return c.setFlags[name]
 }
 
 // GlobalIsSet determines if the global flag was actually set
 func (c *Context) GlobalIsSet(name string) bool {
-	if c.globalSetFlags == nil {
+	ctx := c
-		c.globalSetFlags = make(map[string]bool)
+	if ctx.parentContext != nil {
-		ctx := c
+		ctx = ctx.parentContext
-		if ctx.parentContext != nil {
+	}
-			ctx = ctx.parentContext
+
-		}
+	for ; ctx != nil; ctx = ctx.parentContext {
-		for ; ctx != nil && c.globalSetFlags[name] == false; ctx = ctx.parentContext {
+		if ctx.IsSet(name) {
-			ctx.flagSet.Visit(func(f *flag.Flag) {
+			return true
-				c.globalSetFlags[f.Name] = true
-			})
 		}
 	}
-	return c.globalSetFlags[name]
+	return false
 }
 
 // FlagNames returns a slice of flag names used in this context.

context_test.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"flag"
+	"os"
 	"testing"
 	"time"
 )
@@ -180,6 +181,33 @@ func TestContext_IsSet(t *testing.T) {
 	expect(t, c.IsSet("myflagGlobal"), false)
 }
 
+// XXX Corresponds to hack in context.IsSet for flags with EnvVar field
+// Should be moved to `flag_test` in v2
+func TestContext_IsSet_fromEnv(t *testing.T) {
+	var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool
+
+	os.Clearenv()
+	os.Setenv("APP_TIMEOUT_SECONDS", "15.5")
+	a := App{
+		Flags: []Flag{
+			Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"},
+			Float64Flag{Name: "no-env-var, n"},
+		},
+		Action: func(ctx *Context) error {
+			timeoutIsSet = ctx.IsSet("timeout")
+			tIsSet = ctx.IsSet("t")
+			noEnvVarIsSet = ctx.IsSet("no-env-var")
+			nIsSet = ctx.IsSet("n")
+			return nil
+		},
+	}
+	a.Run([]string{"run"})
+	expect(t, timeoutIsSet, true)
+	expect(t, tIsSet, true)
+	expect(t, noEnvVarIsSet, false)
+	expect(t, nIsSet, false)
+}
+
 func TestContext_GlobalIsSet(t *testing.T) {
 	set := flag.NewFlagSet("test", 0)
 	set.Bool("myflag", false, "doc")
@@ -199,6 +227,38 @@ func TestContext_GlobalIsSet(t *testing.T) {
 	expect(t, c.GlobalIsSet("bogusGlobal"), false)
 }
 
+// XXX Corresponds to hack in context.IsSet for flags with EnvVar field
+// Should be moved to `flag_test` in v2
+func TestContext_GlobalIsSet_fromEnv(t *testing.T) {
+	var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool
+
+	os.Clearenv()
+	os.Setenv("APP_TIMEOUT_SECONDS", "15.5")
+	a := App{
+		Flags: []Flag{
+			Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"},
+			Float64Flag{Name: "no-env-var, n"},
+		},
+		Commands: []Command{
+			{
+				Name: "hello",
+				Action: func(ctx *Context) error {
+					timeoutIsSet = ctx.GlobalIsSet("timeout")
+					tIsSet = ctx.GlobalIsSet("t")
+					noEnvVarIsSet = ctx.GlobalIsSet("no-env-var")
+					nIsSet = ctx.GlobalIsSet("n")
+					return nil
+				},
+			},
+		},
+	}
+	a.Run([]string{"run", "hello"})
+	expect(t, timeoutIsSet, true)
+	expect(t, tIsSet, true)
+	expect(t, noEnvVarIsSet, false)
+	expect(t, nIsSet, false)
+}
+
 func TestContext_NumFlags(t *testing.T) {
 	set := flag.NewFlagSet("test", 0)
 	set.Bool("myflag", false, "doc")