diff --git a/README.md b/README.md index b9e3287..ddd643a 100644 --- a/README.md +++ b/README.md @@ -118,15 +118,21 @@ built on top of them. Anything else (a `map`, an array, ...) is reported as an error rather than silently skipped. - **slices** are separated by a space by default, change it with - `e := ecp.New(); e.Advance.SplitChar = ","` + `e := ecp.New(); e.Advance.SplitChar = ","`. The default separator + collapses repeats, so `a b` is two elements; a separator you choose is + taken literally, empty elements and all - **durations** accept everything `time.ParseDuration` does, plus `Xd` for X days: `10s`, `5m`, `6d` -- **integers** also accept `1e3` and `1,000` notation +- **integers** also accept `1e3` and `1,000` notation. Slice elements do + not: there `1,2` is far more likely to be the wrong separator than the + number 12, so it is reported instead of quietly parsed - **pointers** (`*int`, `*time.Duration`, ...) only get their default when they are nil, which makes "unset" and "set to the zero value" distinguishable - **pointers to a struct** are optional sections: they are walked into like - a plain struct and only allocated when one of their fields is set + a plain struct and only allocated when one of their fields is actually + set, whether from the environment or from a `default` tag. A section + nothing was said about stays nil An environment variable set to an empty value is treated as unset, so a field keeps its default. diff --git a/parse.go b/parse.go index 2893d86..24619cd 100644 --- a/parse.go +++ b/parse.go @@ -6,14 +6,32 @@ import ( "strings" ) -// split cuts a value into slice elements. With the default whitespace -// separator, repeated separators are collapsed, so "a b" yields two -// elements instead of three (one of them empty and unparsable). +// split cuts a value into slice elements. +// +// Only the default separator collapses repeats, so "a b" yields two +// elements instead of three (one of them empty and unparsable). Any +// separator the caller chose is taken literally, including a tab or a +// newline, which a "is it whitespace" test used to swallow. func (e *ECP) split(v string) []string { - if strings.TrimSpace(e.Advance.SplitChar) == "" { - return strings.Fields(v) + sep := e.Advance.SplitChar + if sep == "" { + // an empty separator would make strings.Split cut between every + // rune, which is never what the caller meant + sep = space + } + + parts := strings.Split(v, sep) + if sep != space { + return parts + } + + collapsed := parts[:0] + for _, p := range parts { + if p != "" { + collapsed = append(collapsed, p) + } } - return strings.Split(v, e.Advance.SplitChar) + return collapsed } // parseSlice supports slices of string, bool, int, int8, int16, int32, diff --git a/range.go b/range.go index 532cc27..72c0067 100644 --- a/range.go +++ b/range.go @@ -94,6 +94,19 @@ type roOption struct { // type (type Node struct{ Next *Node }) stops instead of recursing // until the stack blows up visiting map[reflect.Type]bool + // filled reports whether any field was actually assigned. An + // optional section needs to know that, since a section filled + // entirely with zero values (PORT=0) is still a section that was + // asked for, and testing the result for zero cannot tell the two + // apart. + filled *bool +} + +// markFilled records that a field was assigned during this walk +func (o roOption) markFilled() { + if o.filled != nil { + *o.filled = true + } } func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { @@ -158,6 +171,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { // set value via self-defined function if opts.find == "" && e.Advance.SetValue != nil && e.Advance.SetValue(info.tag, field, v) { + opts.markFilled() continue } @@ -170,6 +184,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { prefix: prefix, find: opts.find, visiting: opts.visiting, + filled: opts.filled, }) if err != nil { return reflect.Value{}, err @@ -197,6 +212,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { if err := e.setPointer(field, v); err != nil { return field, fmt.Errorf("convert %s error: %w", keyName, err) } + opts.markFilled() case reflect.Slice: if !field.IsNil() && !exist { @@ -205,6 +221,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { if err := e.parseSlice(v, field); err != nil { return field, fmt.Errorf("convert %s error: %w", keyName, err) } + opts.markFilled() default: // a value already set by the caller wins over the default, @@ -219,6 +236,7 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { if err := setValue(field, value); err != nil { return field, fmt.Errorf("convert %s error: %w", keyName, err) } + opts.markFilled() } } @@ -227,8 +245,9 @@ func (e *ECP) rangeOver(opts roOption) (reflect.Value, error) { // rangeOverPointer walks into a pointer to a struct, that is an optional // config section. A nil section is filled through a temporary value and -// only allocated when something was actually set, so that an untouched -// optional section stays nil. +// only allocated when one of its fields was actually assigned, so that an +// untouched optional section stays nil while a section explicitly asked +// for is allocated even when every value in it is zero. func (e *ECP) rangeOverPointer(field reflect.Value, structName string, tag reflect.StructTag, opts roOption) (reflect.Value, error) { @@ -247,20 +266,27 @@ func (e *ECP) rangeOverPointer(field reflect.Value, structName string, target = reflect.New(elemType) } + filled := false found, err := e.rangeOver(roOption{ target: target.Elem(), setDef: opts.setDef, prefix: e.BuildKey(opts.prefix, structName, tag), find: opts.find, visiting: opts.visiting, + filled: &filled, }) if err != nil { return reflect.Value{}, err } - if field.IsNil() && !target.Elem().IsZero() { + if !filled { + return found, nil + } + if field.IsNil() { field.Set(target) } + // an allocated or updated section fills the struct holding it + opts.markFilled() return found, nil } @@ -281,7 +307,11 @@ func parseScientific(v string) (string, error) { } n, err := strconv.Atoi(v[index+1:]) if err != nil { - return "", err + // not scientific notation at all, just a value that happens to + // contain an "e". Handing it back unchanged lets the caller + // report it as a whole ("hello"), instead of blaming the + // fragment after the e ("llo"). + return v, nil } // a negative exponent would be silently ignored by the expansion // below (e.g. "1e-3" -> "1"), which is worse than an error diff --git a/regression_test.go b/regression_test.go index 573c6f0..2255064 100644 --- a/regression_test.go +++ b/regression_test.go @@ -450,3 +450,93 @@ func TestSameTypeSections(t *testing.T) { } } } + +// a separator the caller chose is taken literally; only the default one +// collapses repeats. A "is the separator whitespace" test used to route +// a deliberate tab through strings.Fields and split on spaces too. +func TestCustomWhitespaceSplitChar(t *testing.T) { + for _, tc := range []struct { + sep string + in string + want []string + }{ + {"\t", "a b\tc d", []string{"a b", "c d"}}, + {"\n", "a b\nc d", []string{"a b", "c d"}}, + {",", "a b,c d", []string{"a b", "c d"}}, + {" ", " a b ", []string{"a", "b"}}, // the default still collapses + {"", "a b", []string{"a", "b"}}, // unset falls back to the default + } { + e := New() + e.Advance.SplitChar = tc.sep + c := &struct { + S []string `env:"SPLIT_S"` + }{} + os.Setenv("SPLIT_S", tc.in) + err := e.Parse(c) + os.Unsetenv("SPLIT_S") + if err != nil { + t.Errorf("sep %q: %v", tc.sep, err) + continue + } + if strings.Join(c.S, "|") != strings.Join(tc.want, "|") { + t.Errorf("sep %q: got %q, want %q", tc.sep, c.S, tc.want) + } + } +} + +// a section is allocated when one of its fields was assigned, even if +// every value in it is zero. Testing the result for zero instead used to +// leave the section nil for an explicit PORT=0. +func TestPointerSectionExplicitZero(t *testing.T) { + type sub struct { + Port int + Name string + } + type conf struct { + P *sub `yaml:"p"` + Empty *sub `yaml:"empty"` + } + + withEnv(t, "P_PORT", "0") + c := &conf{} + if err := Parse(c); err != nil { + t.Fatal(err) + } + if c.P == nil { + t.Error("a section with an explicit value must be allocated") + } else if c.P.Port != 0 { + t.Errorf("port: %d", c.P.Port) + } + // and a section nothing was said about stays nil + if c.Empty != nil { + t.Errorf("untouched section should stay nil, got %+v", c.Empty) + } + + // every key List advertises has to be readable back after Parse + for _, item := range List(conf{}) { + key := strings.Split(item, "=")[0] + if _, err := Get(c, key); err != nil && strings.HasPrefix(key, "P_") { + t.Errorf("listed key %s is not gettable: %v", key, err) + } + } +} + +// a value that merely contains an "e" is reported as a whole, it used to +// be blamed on the fragment after the e +func TestNonNumericValueErrorMessage(t *testing.T) { + for _, in := range []string{"not-a-number", "hello"} { + c := &struct { + N int `env:"MSG_N"` + }{} + os.Setenv("MSG_N", in) + err := Parse(c) + os.Unsetenv("MSG_N") + if err == nil { + t.Errorf("%q should not parse", in) + continue + } + if !strings.Contains(err.Error(), in) { + t.Errorf("error for %q does not mention it: %v", in, err) + } + } +}