Parse: refactor flag name matching and duplicate detection behavior

errors.go

@@ -14,6 +14,10 @@ var (
 	// add a flag with the same name as a pre-existing flag.
 	ErrDuplicateFlag = errors.New("duplicate flag")
 
+	// ErrAmbiguousFlag should be returned when a flag name is ambiguous and
+	// matches more than one defined flag.
+	ErrAmbiguousFlag = errors.New("ambiguous flag")
+
 	// ErrNotParsed may be returned by flag set methods which require the flag
 	// set to have been successfully parsed, and that condition isn't satisfied.
 	ErrNotParsed = errors.New("not parsed")

ffenv/ffenv.go

@@ -10,16 +10,29 @@ import (
 	"strings"
 )
 
-// Parse is a parser for .env files. Each line is tokenized on the first `=`
-// character. The first token is interpreted as the env var representation of
-// the flag name, and the second token is interpreted as the value. Both tokens
-// are trimmed of leading and trailing whitespace. If the value is "double
-// quoted", control characters like `\n` are expanded. Lines beginning with `#`
-// are interpreted as comments. End-of-line comments are not supported.
+// Parse is a parser for .env files.
 //
-// The parser respects the [ff.WithEnvVarPrefix] option. For example, if parse
-// is called with an env var prefix MYPROG, then both FOO=bar and MYPROG_FOO=bar
-// would set a flag named foo.
+// Each line in the .env file is tokenized on the first `=` character. The first
+// token is interpreted as the env var representation of the flag name, and the
+// second token is interpreted as the value. Both tokens are trimmed of leading
+// and trailing whitespace. If the value is "double quoted", control characters
+// like `\n` are expanded. Lines beginning with `#` are interpreted as comments.
+// End-of-line comments are not supported.
+//
+// Parse options related to environment variables, like [ff.WithEnvVarPrefix],
+// [ff.WithEnvVarShortNames], and [ff.WithEnvVarCaseSensitive], also apply to
+// .env files. For example, WithEnvVarPrefix("MYPROG") means that the keys in an
+// .env file must begin with MYPROG_.
+//
+// If for any reason any key in an .env file matches multiple flags, parse will
+// return [ff.ErrDuplicateFlag]. This can happen if you have flags with names
+// that differ only in capitalization, e.g. `-v` and `-V`. If you want to
+// support setting these flags from an .env file, either use discrete long
+// names, or [ff.WithEnvVarCaseSensitive].
+//
+// Using the .env config file parser doesn't automatically enable parsing of
+// actual environment variables. To do so, callers must still explicitly provide
+// e.g. [ff.WithEnvVars] or [ff.WithEnvVarPrefix].
 func Parse(r io.Reader, set func(name, value string) error) error {
 	s := bufio.NewScanner(r)
 	for s.Scan() {

ffenv/ffenv_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/peterbourgon/ff/v4"
+	"github.com/peterbourgon/ff/v4/ffenv"
 	"github.com/peterbourgon/ff/v4/fftest"
 )
 
@@ -18,50 +19,211 @@ func TestEnvFileParser(t *testing.T) {
 			Want:       fftest.Vars{},
 		},
 		{
-			ConfigFile: "testdata/basic.env",
-			Want:       fftest.Vars{S: "bar", I: 99, B: true, D: time.Hour},
+			ConfigFile:   "testdata/basic.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Want:         fftest.Vars{S: "bar", I: 99, B: true, D: time.Hour},
 		},
 		{
-			ConfigFile: "testdata/prefix.env",
-			Options:    []ff.Option{ff.WithEnvVarPrefix("MYPROG")},
-			Want:       fftest.Vars{S: "bingo", I: 123},
+			ConfigFile:   "testdata/prefix.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Options:      []ff.Option{ff.WithEnvVarPrefix("MYPROG")},
+			Want:         fftest.Vars{S: "bingo", I: 123},
 		},
 		{
-			ConfigFile: "testdata/prefix-undef.env",
-			Options:    []ff.Option{ff.WithEnvVarPrefix("MYPROG"), ff.WithConfigIgnoreUndefinedFlags()},
-			Want:       fftest.Vars{S: "bango", I: 9},
+			ConfigFile:   "testdata/prefix-undef.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Options:      []ff.Option{ff.WithEnvVarPrefix("MYPROG"), ff.WithConfigIgnoreUndefinedFlags()},
+			Want:         fftest.Vars{S: "bango", I: 9},
 		},
 		{
-			ConfigFile: "testdata/quotes.env",
-			Want:       fftest.Vars{S: "", I: 32, X: []string{"1", "2 2", "3 3 3"}},
+			ConfigFile:   "testdata/quotes.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Want:         fftest.Vars{S: "", I: 32, X: []string{"1", "2 2", "3 3 3"}},
 		},
 		{
-			ConfigFile: "testdata/no-value.env",
-			Want:       fftest.Vars{WantParseErrorString: "D: parse error"},
+			ConfigFile:   "testdata/no-value.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Want:         fftest.Vars{WantParseErrorString: "DUR: parse error"},
 		},
 		{
-			ConfigFile: "testdata/spaces.env",
-			Want:       fftest.Vars{X: []string{"1", "2", "3", "4", "5", " 6", " 7 ", " 8 ", "9"}},
+			ConfigFile:   "testdata/spaces.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Want:         fftest.Vars{X: []string{"1", "2", "3", "4", "5", " 6", " 7 ", " 8 ", "9"}},
 		},
 		{
-			ConfigFile: "testdata/newlines.env",
-			Want:       fftest.Vars{S: "one\ntwo\nthree\n\n", X: []string{`A\nB\n\n`}},
+			ConfigFile:   "testdata/newlines.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Want:         fftest.Vars{S: "one\ntwo\nthree\n\n", X: []string{`A\nB\n\n`}},
 		},
 		{
-			ConfigFile: "testdata/capitalization.env",
-			Want:       fftest.Vars{S: "hello", I: 12345},
+			ConfigFile:   "testdata/comments.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Want:         fftest.Vars{S: "abc # def"},
 		},
 		{
-			ConfigFile: "testdata/comments.env",
-			Want:       fftest.Vars{S: "abc # def"},
+			ConfigFile:   "testdata/short.env",
+			Constructors: fftest.DefaultConstructors,
+			Options:      []ff.Option{ff.WithEnvVarShortNames()},
+			Want:         fftest.Vars{S: "hello", I: 99, D: 8 * time.Millisecond},
+		},
+		{
+			ConfigFile:   "testdata/case-sensitive.env",
+			Constructors: []fftest.Constructor{fftest.CoreConstructor},
+			Options:      []ff.Option{ff.WithEnvVarPrefix("MYPREFIX"), ff.WithEnvVarCaseSensitive(), ff.WithConfigIgnoreUndefinedFlags()},
+			Want:         fftest.Vars{S: "hello", I: 12345, D: 1*time.Minute + 30*time.Second},
 		},
 	}
 
 	for i := range testcases {
+		testcases[i].Constructors = []fftest.Constructor{
+			fftest.CoreConstructor,
+		}
 		if testcases[i].Name == "" {
 			testcases[i].Name = filepath.Base(testcases[i].ConfigFile)
 		}
 	}
 
 	testcases.Run(t)
 }
+
+func TestAmbiguous(t *testing.T) {
+	t.Parallel()
+
+	fs := ff.NewFlagSet(t.Name())
+	verboseFlag := fs.Bool('v', "verbose", "verbose output")
+	versionFlag := fs.Bool('V', "version", "print version")
+
+	for _, tc := range []struct {
+		name        string
+		options     []ff.Option
+		wantErr     bool
+		wantVerbose bool
+		wantVersion bool
+	}{
+		{
+			// With just the config file parser, env vars aren't enabled, so we
+			// don't do duplicate detection up-front. And because we don't
+			// provide WithEnvVarShortNames(), we're only using long names to
+			// populate env2flags, so there is no v/V ambiguity. Also important:
+			// the `V=true` doesn't match to anything in the env, but it *does*
+			// match to a (short) flag name, because we didn't provide
+			// WithConfigIgnoreFlagNames().
+			name:        "ambiguous-1.env WithConfigFile",
+			options:     []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env")},
+			wantErr:     false,
+			wantVerbose: true,
+			wantVersion: true, // V=true should match to `-V, --version`
+		},
+		{
+			// This is the same as above, but WithConfigIgnoreFlagNames() means
+			// the `V=true` doesn't match to anything any more, and because we
+			// didn't provide WithConfigIgnoreUndefinedFlags() that's an error.
+			name:    "ambiguous-1.env WithConfigIgnoreFlagNames",
+			options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithConfigIgnoreFlagNames()},
+			wantErr: true,
+		},
+		{
+			// Same as above, but passing WithConfigIgnoreUndefinedFlags() means
+			// it can now ignore `V=true` and succeed.
+			name:        "ambiguous-1.env WithConfigIgnoreFlagNames",
+			options:     []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithConfigIgnoreFlagNames(), ff.WithConfigIgnoreUndefinedFlags()},
+			wantErr:     false,
+			wantVerbose: true,
+			wantVersion: false,
+		},
+		{
+			// WithEnvVarShortNames() by itself doesn't enable env var parsing,
+			// and so doesn't trigger duplicate detection.
+			name:        "ambiguous-1.env WithEnvVarShortNames",
+			options:     []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames()},
+			wantErr:     false,
+			wantVerbose: true,
+			wantVersion: true,
+		},
+		{
+			// WithEnvVarShortNames() combined with WithEnvVars() triggers
+			// duplicate detection up-front. Without WithEnvVarCaseSensitive(),
+			// `-v` and `-V` both map to `V` and so are ambiguous, which results
+			// in an error.
+			name:    "ambiguous-1.env WithEnvVarShortNames WithEnvVars",
+			options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames(), ff.WithEnvVars()},
+			wantErr: true,
+		},
+		{
+			// Same as above, but passing WithEnvVarCaseSensitive() means that
+			// `-v` and `-V` are no longer ambiguous, and that `V=true` now
+			// matches to `-V, --version`. But it also means that `VERSION=true`
+			// is invalid, since it would need to be `version=true`. So, another
+			// error.
+			name:    "ambiguous-1.env WithEnvVarShortNames WithEnvVarCaseSensitive",
+			options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames(), ff.WithEnvVarCaseSensitive()},
+			wantErr: true,
+		},
+		{
+			// But if we ignore undefined flags, then the parse can succeed.
+			name:        "ambiguous-1.env WithEnvVarShortNames WithEnvVarCaseSensitive WithConfigIgnoreUndefinedFlags",
+			options:     []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames(), ff.WithEnvVarCaseSensitive(), ff.WithConfigIgnoreUndefinedFlags()},
+			wantErr:     false,
+			wantVerbose: false,
+			wantVersion: true,
+		},
+		{
+			name:        "ambiguous-2.env matches both short names",
+			options:     []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env")},
+			wantErr:     false,
+			wantVerbose: true,
+			wantVersion: true,
+		},
+		{
+			name:    "ambiguous-2.env WithConfigIgnoreFlagNames no match",
+			options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames()},
+			wantErr: true,
+		},
+		{
+			name:    "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVars no match",
+			options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVars()},
+			wantErr: true,
+		},
+		{
+			name:    "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVars WithEnvVarShortNames duplicate",
+			options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVars(), ff.WithEnvVarShortNames()},
+			wantErr: true,
+		},
+		{
+			name:    "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVarShortNames ambiguous",
+			options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVarShortNames()},
+			wantErr: true,
+		},
+		{
+			name:        "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVarShortNames WithEnvVarCaseSensitive OK",
+			options:     []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVarShortNames(), ff.WithEnvVarCaseSensitive()},
+			wantErr:     false,
+			wantVerbose: true,
+			wantVersion: true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			fs.Reset()
+
+			options := append([]ff.Option{ff.WithConfigFileParser(ffenv.Parse)}, tc.options...)
+
+			err := ff.Parse(fs, []string{}, options...)
+			t.Logf("--verbose=%v --version=%v error=%v", *verboseFlag, *versionFlag, err)
+
+			switch {
+			case tc.wantErr:
+				if want, have := tc.wantErr, err != nil; want != have {
+					t.Errorf("error: want %v, have %v", want, have)
+				}
+
+			default:
+				if want, have := tc.wantVerbose, *verboseFlag; want != have {
+					t.Errorf("verbose: want %v, have %v", want, have)
+				}
+				if want, have := tc.wantVersion, *versionFlag; want != have {
+					t.Errorf("version: want %v, have %v", want, have)
+				}
+			}
+		})
+	}
+}

ffenv/testdata/ambiguous-1.env

@@ -0,0 +1,6 @@
+# should be OK
+VERBOSE=true
+
+# matches flag name -V by default
+# with IgnoreFlagNames -> error
+V=true

ffenv/testdata/ambiguous-2.env

@@ -0,0 +1,2 @@
+V=true
+v=true

ffenv/testdata/basic.env

@@ -1,4 +1,4 @@
-S=bar
-I=99
-B=true
-D=1h
+STR=bar
+INT=99
+BFLAG=true
+DUR=1h

ffenv/testdata/case-sensitive.env

@@ -0,0 +1,7 @@
+# works with CaseSensitive, fails without
+MYPREFIX_str=hello
+MYPREFIX_int=12345
+MYPREFIX_dur=1m30s
+
+# works with IgnoreUndefined, fails without
+MYPREFIX_FLT=3.33

ffenv/testdata/comments.env

@@ -3,4 +3,4 @@
 	# comment 3
 
 # S will be set to `abc # def`
-S=abc # def
+STR=abc # def

ffenv/testdata/newlines.env

@@ -1,2 +1,2 @@
-S="one\ntwo\nthree\n\n"
-X=A\nB\n\n
+STR="one\ntwo\nthree\n\n"
+XXX=A\nB\n\n

ffenv/testdata/no-value.env

@@ -1,3 +1,3 @@
-I=32
-D=
-S=this is fine
+INT=32
+DUR=
+STR=this is fine

ffenv/testdata/prefix-undef.env

@@ -1,6 +1,6 @@
 
 
-MYPROG_I=9
-OTHERPREFIX_B=true
-MYPROG_S=bango
-D=32m
+MYPROG_INT=9
+OTHERPREFIX_BFLAG=true
+MYPROG_STR=bango
+DUR=32m

ffenv/testdata/prefix.env

@@ -1,4 +1,4 @@
 
-MYPROG_S=bingo
+MYPROG_STR=bingo
 
-MYPROG_I=123
+MYPROG_INT=123

ffenv/testdata/quotes.env

@@ -1,5 +1,5 @@
-S=""
-X=1
-X=2 2
-X="3 3 3"
-I="32"
+STR=""
+XXX=1
+XXX=2 2
+XXX="3 3 3"
+INT="32"

ffenv/testdata/short.env

@@ -0,0 +1,5 @@
+S=hello
+I=99
+
+# long names are still valid
+DUR=8ms

ffenv/testdata/spaces.env

@@ -1,9 +1,9 @@
-X = 1
-X= 2
-X =3
-X=    4
- X   = 5
-X=" 6"
-X= " 7 "
-X = " 8 "
-	X	=	9
+XXX = 1
+XXX= 2
+XXX =3
+XXX=    4
+ XXX   = 5
+XXX=" 6"
+XXX= " 7 "
+XXX = " 8 "
+	XXX	=	9