fix: panics on named types, unbounded exponent, ignored "-" tag

.github/workflows/go.yml

@@ -11,15 +11,41 @@ on:
 
 jobs:
 
-  build:
+  test:
     runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        go-version: [ "1.19", "1.21", "1.23", "stable" ]
+
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
 
     - name: Set up Go
-      uses: actions/setup-go@v3
+      uses: actions/setup-go@v5
       with:
-        go-version: 1.19
+        go-version: ${{ matrix.go-version }}
+
+    - name: Vet
+      run: go vet ./...
 
     - name: Test
-      run: go test -v ./...
+      run: go test -race -coverprofile=coverage.out -covermode=atomic ./...
+
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v4
+
+    - name: Set up Go
+      uses: actions/setup-go@v5
+      with:
+        go-version: stable
+
+    - name: Gofmt
+      run: |
+        unformatted=$(gofmt -l .)
+        if [ -n "$unformatted" ]; then
+          echo "These files need gofmt:"
+          echo "$unformatted"
+          exit 1
+        fi

README.md

@@ -12,7 +12,7 @@ The environment config keys can be auto generated or set by the `yaml` or `env`
 The only thing you should do is importing this package, and `Parse` your config.
 
 [![Go Report Card](https://goreportcard.com/badge/github.com/wrfly/ecp)](https://goreportcard.com/report/github.com/wrfly/ecp)
-[![Build Status](https://travis-ci.org/wrfly/ecp.svg?branch=master)](https://travis-ci.org/wrfly/ecp)
+[![Go](https://github.com/wrfly/ecp/actions/workflows/go.yml/badge.svg)](https://github.com/wrfly/ecp/actions/workflows/go.yml)
 [![GoDoc](https://godoc.org/github.com/wrfly/ecp?status.svg)](https://godoc.org/github.com/wrfly/ecp)
 [![license](https://img.shields.io/github/license/wrfly/ecp.svg)](https://github.com/wrfly/ecp/blob/master/LICENSE)
 
@@ -44,7 +44,7 @@ func main() {
     // set some env
     envs := map[string]string{
         "LOGLEVEL": "info",
-        "PORT":         "1234",
+        "PORT":     "1234",
     }
     for k, v := range envs {
         fmt.Printf("export %s=%s\n", k, v)
@@ -53,16 +53,15 @@ func main() {
     fmt.Println()
 
     // then parse configuration from environments
-    if err := ecp.Parse(config, "ECP"); err != nil {
+    if err := ecp.Parse(config); err != nil {
         panic(err)
     }
     fmt.Printf("new log level: [ %s ], port: [ %d ]\n",
         config.LogLevel, config.Port)
     fmt.Println()
 
     // and list all the env keys
-    envLists := ecp.List(config, "ecp")
-    for _, k := range envLists {
+    for _, k := range ecp.List(config) {
         fmt.Println(k)
     }
 }
@@ -81,3 +80,69 @@ new log level: [ info ], port: [ 1234 ]
 LOGLEVEL=debug
 PORT=
 ```
+
+`Parse` needs a pointer to a struct, `ecp.Parse(config)` on a plain struct
+returns an error instead of quietly filling in nothing.
+
+## Keys
+
+The key of a field is built from the name of the struct it lives in and the
+name of the field itself, upper cased and joined with `_`. A `yaml` or
+`json` tag replaces the field name, and an `env` tag replaces the whole key:
+
+```go
+type Conf struct {
+    LogLevel string                     // LOGLEVEL
+    Redis    struct {
+        Host string `yaml:"host"`       // REDIS_HOST
+        Port int    `env:"REDIS_PORT"`  // REDIS_PORT
+    } `yaml:"redis"`
+    Secret   string `yaml:"-"`          // ignored
+}
+```
+
+Pass a prefix to namespace them: `ecp.Parse(&config, "APP")` reads
+`APP_LOGLEVEL` and `APP_REDIS_HOST`. A key coming from an `env` tag is
+taken as is, so `REDIS_PORT` stays `REDIS_PORT`. The same prefix goes to
+`List` and to the `Get` helpers.
+
+## Values
+
+`Parse` sets a field when the environment key exists, otherwise it falls
+back to the `default` tag. A default never overwrites a value the caller
+already set, an environment value always does.
+
+Supported types are `string`, `bool`, all sized `int`/`uint` types,
+`float32`, `float64`, `time.Duration`, slices of those, and named types
+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 = ","`
+- **durations** accept everything `time.ParseDuration` does, plus `Xd`
+  for X days: `10s`, `5m`, `6d`
+- **integers** also accept `1e3` and `1,000` notation
+- **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
+
+An environment variable set to an empty value is treated as unset, so a
+field keeps its default.
+
+## Advanced
+
+`ecp.New()` returns a parser whose behaviour can be changed:
+
+```go
+e := ecp.New()
+e.BuildKey = func(structure, field string, tag reflect.StructTag) string { ... }
+e.LookupValue = func(key string) (string, bool) { ... }
+e.Advance.SplitChar = ","
+e.Advance.SetValue = func(tag reflect.StructTag, field reflect.Value, val string) bool { ... }
+```
+
+`LookupValue` is what makes it possible to read from something other than
+the environment, and `SetValue` takes over the conversion of a field,
+returning true when it handled it.

ecp.go

@@ -5,88 +5,137 @@ package ecp
 import (
 	"fmt"
 	"reflect"
+	"strconv"
 	"strings"
 )
 
-type ecp struct {
-	BuildKey    BuildKeyFunc
+// ECP is an environment config parser. Create one with New when the
+// default behaviour has to be changed, or use the package level Parse,
+// List and Get functions to work with the default one.
+type ECP struct {
+	// BuildKey builds the environment key of a field
+	BuildKey BuildKeyFunc
+	// LookupValue returns the value of a key and whether it exists
 	LookupValue LookupValueFunc
 
-	Advance advanceConfig
+	Advance AdvanceConfig
 }
 
-type advanceConfig struct {
+// AdvanceConfig holds the optional knobs of an ECP
+type AdvanceConfig struct {
 	SplitChar string // split slice
 	SetValue  SetValueFunc
 }
 
-var globalEcp = &ecp{
-	BuildKey:    buildKeyFromEnv,
-	LookupValue: lookupValueFromEnv,
-	Advance: advanceConfig{
-		SplitChar: space,
-	},
-}
+var globalEcp = New()
 
 // New ecp object
-func New() *ecp {
-	return &ecp{
+func New() *ECP {
+	return &ECP{
 		BuildKey:    buildKeyFromEnv,
 		LookupValue: lookupValueFromEnv,
-		Advance: advanceConfig{
+		Advance: AdvanceConfig{
 			SplitChar: space,
 		},
 	}
 }
 
-func (e *ecp) Parse(config interface{}, prefix ...string) error {
+// Parse the configuration through environments starting with the
+// prefix (or not), see the package level Parse for the details
+func (e *ECP) Parse(config interface{}, prefix ...string) error {
 	if len(prefix) == 0 {
 		prefix = []string{""}
 	}
-	_, err := e.rangeOver(roOption{config, true, prefix[0], ""})
+
+	// catch the classic Parse(config) instead of Parse(&config): without
+	// a pointer every field is read-only, so Parse would report success
+	// after filling in exactly nothing
+	if value := toValue(config); value.IsValid() &&
+		value.Kind() == reflect.Struct && !value.CanSet() {
+		return fmt.Errorf("config must be a pointer to a struct, got %s", value.Type())
+	}
+
+	_, err := e.rangeOver(roOption{target: config, setDef: true, prefix: prefix[0]})
 	return err
 }
 
-func (e *ecp) List(config interface{}, prefix ...string) []string {
-	list := []string{}
-
+// List all the config environments, see the package level List for
+// the details
+func (e *ECP) List(config interface{}, prefix ...string) []string {
 	if len(prefix) == 0 {
 		prefix = []string{""}
 	}
-	parentName := prefix[0]
+	return e.list(config, prefix[0], make(map[reflect.Type]bool, 1))
+}
+
+func (e *ECP) list(config interface{}, parentName string,
+	visiting map[reflect.Type]bool) []string {
+
+	list := []string{}
 
 	configValue := toValue(config)
 	if !configValue.IsValid() || configValue.Kind() != reflect.Struct {
 		return list
 	}
 	configType := configValue.Type()
+
+	// stop a self referencing type from recursing forever
+	if visiting[configType] {
+		return list
+	}
+	visiting[configType] = true
+	defer delete(visiting, configType)
+
 	for index := 0; index < configValue.NumField(); index++ {
 		if !configType.Field(index).IsExported() {
 			continue
 		}
 		all := e.getAll(getAllOpt{configType, configValue, index, parentName})
-		if all.parent == "-" || all.key == "" {
+		if all.key == "" {
 			continue
 		}
-		switch all.value.Kind() {
-		case reflect.Struct:
+		switch {
+		case all.value.Kind() == reflect.Struct:
 			prefix := e.BuildKey(parentName, all.parent, all.tag)
-			list = append(list, e.List(all.value, prefix)...)
-		default:
-			if strings.Contains(all.defVal, " ") {
-				all.defVal = fmt.Sprintf(`"%s"`, all.defVal)
+			list = append(list, e.list(all.value, prefix, visiting)...)
+
+		case isSection(all.value):
+			// an optional section: list the keys of the pointed-to
+			// struct, a nil pointer still has all of them
+			prefix := e.BuildKey(parentName, all.parent, all.tag)
+			section := all.value
+			if section.IsNil() {
+				section = reflect.New(all.value.Type().Elem())
 			}
-			list = append(list, fmt.Sprintf("%s=%s", all.key, all.defVal))
+			list = append(list, e.list(section.Elem(), prefix, visiting)...)
+
+		case !canSetKind(all.value.Kind()):
+			// maps, arrays, channels... cannot be filled from a string,
+			// so listing a key for them would be misleading
+			continue
+
+		default:
+			list = append(list, fmt.Sprintf("%s=%s", all.key, quoteValue(all.defVal)))
 		}
 	}
 
 	return list
 }
 
-// List function will also fill up the value of the environment key
-// it the "default" tag has value
+// quoteValue quotes a default value that would not survive a round trip
+// through a shell or an env file unquoted
+func quoteValue(v string) string {
+	if strings.ContainsAny(v, " \t\r\n\"'\\`$") {
+		return strconv.Quote(v)
+	}
+	return v
+}
 
-// List all the config environments
+// List all the config environments.
+//
+// The value of each key is the one from the "default" tag, empty if the
+// field has no default. Fields tagged with `env:"-"`, `yaml:"-"` or
+// `json:"-"` are skipped.
 func List(config interface{}, prefix ...string) []string {
 	return globalEcp.List(config, prefix...)
 }
@@ -109,9 +158,14 @@ func List(config interface{}, prefix ...string) []string {
 //	type config struct {
 //	    One   string   `default:"1"`
 //	    Two   int      `default:"2"`
-//	    Three []string `default:"1,2,3"`
+//	    Three []string `default:"1 2 3"`
 //	}
 //	c := &config{}
+//
+// Slice values are separated by Advance.SplitChar, a space by default.
+//
+// config must be a pointer to a struct, otherwise Parse returns an error
+// instead of silently doing nothing.
 func Parse(config interface{}, prefix ...string) error {
 	return globalEcp.Parse(config, prefix...)
 }

ecp_test.go

@@ -124,16 +124,16 @@ func TestList(t *testing.T) {
 	})
 
 	t.Run("with get key", func(t *testing.T) {
+		// reset get key function
+		defer func() { globalEcp.BuildKey = buildKeyFromEnv }()
+
 		globalEcp.BuildKey = func(parentName, structName string, tag reflect.StructTag) (key string) {
 			return strings.ToLower(parentName) + "." + strings.ToLower(structName)
 		}
 		list := List(config)
 		for _, key := range list[18:27] {
 			fmt.Printf("%s\n", key)
 		}
-
-		// reset get key function
-		globalEcp.BuildKey = buildKeyFromEnv
 	})
 }
 
@@ -266,8 +266,11 @@ func TestGetKeyLookupValue(t *testing.T) {
 		}
 		return structName
 	}
+	// both hooks have to be restored, leaving the lookup mock behind
+	// makes every test running after this one read from it
 	defer func() {
 		globalEcp.BuildKey = buildKeyFromEnv
+		globalEcp.LookupValue = lookupValueFromEnv
 	}()
 
 	globalEcp.LookupValue = func(key string) (value string, exist bool) {