fix: honour a chosen separator, allocate explicitly-zero sections

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.

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,

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

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)
+		}
+	}
+}