preventive coding

.github/workflows/test.yml

@@ -29,7 +29,7 @@ jobs:
         run: go test -v -shuffle=on -count=10 -race ./...
         working-directory: provider/file
       - name: Test (file)
-        run: go test -v -shuffle=on ./...
+        run: go test -v ./...
         working-directory: provider/file
       - name: Race Test (pflag)
         run: go test -v -shuffle=on -count=10 -race ./...

config.go

@@ -10,6 +10,7 @@
 	"log/slog"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/go-viper/mapstructure/v2"
 
@@ -33,26 +34,32 @@
 }
 
 type provider struct {
-	values  map[string]any
-	watcher Watcher
+	loader Loader
+	values map[string]any
 }
 
 // New creates a new Config with the given Option(s).
 func New(opts ...Option) *Config {
 	option := &options{
-		delimiter: ".",
-		tagName:   "konf",
-		decodeHook: mapstructure.ComposeDecodeHookFunc(
-			mapstructure.StringToTimeDurationHookFunc(),
-			mapstructure.StringToSliceHookFunc(","),
-			mapstructure.TextUnmarshallerHookFunc(),
-		),
 		values:    make(map[string]any),
 		onChanges: make(map[string][]func(*Config)),
 	}
 	for _, opt := range opts {
 		opt(option)
 	}
+	if option.delimiter == "" {
+		option.delimiter = "."
+	}
+	if option.tagName == "" {
+		option.tagName = "konf"
+	}
+	if option.decodeHook == nil {
+		option.decodeHook = mapstructure.ComposeDecodeHookFunc(
+			mapstructure.StringToTimeDurationHookFunc(),
+			mapstructure.StringToSliceHookFunc(","),
+			mapstructure.TextUnmarshallerHookFunc(),
+		)
+	}
 
 	return (*Config)(option)
 }
@@ -61,10 +68,11 @@
 // Each loader takes precedence over the loaders before it.
 //
 // This method can be called multiple times but it is not concurrency-safe.
+// It panics if any loader is nil.
 func (c *Config) Load(loaders ...Loader) error {
-	for _, loader := range loaders {
+	for i, loader := range loaders {
 		if loader == nil {
-			continue
+			panic(fmt.Sprintf("cannot load config from nil loader at loaders[%d]", i))
 		}
 
 		values, err := loader.Load()
@@ -73,14 +81,12 @@
 		}
 		maps.Merge(c.values, values)
 
-		// Merged to empty map to convert to lower case.
 		provider := &provider{
+			loader: loader,
 			values: make(map[string]any),
 		}
+		// Merged to empty map to convert to lower case.
 		maps.Merge(provider.values, values)
-		if w, ok := loader.(Watcher); ok {
-			provider.watcher = w
-		}
 		c.providers = append(c.providers, provider)
 
 		slog.Info(
@@ -97,12 +103,19 @@
 // WARNING: All loaders passed in Load after calling Watch do not get watched.
 //
 // It only can be called once. Call after first has no effects.
+// It panics if ctx is nil.
 func (c *Config) Watch(ctx context.Context) error { //nolint:cyclop,funlen,gocognit
-	initialized := true
+	if ctx == nil {
+		panic("cannot watch change with nil context")
+	}
+
+	watched := true
 	c.watchOnce.Do(func() {
-		initialized = false
+		watched = false
 	})
-	if initialized {
+	if watched {
+		slog.Warn("Config has been watched, call Watch again has no effects.")
+
 		return nil
 	}
 
@@ -124,9 +137,32 @@
 					maps.Merge(values, w.values)
 				}
 				c.values = values
+				slog.Info("Configuration has been updated with change.")
 
-				for _, onChange := range onChanges {
-					onChange(c)
+				if len(onChanges) > 0 {
+					func() {
+						ctx, cancel = context.WithTimeout(context.Background(), time.Minute)
+						defer cancel()
+
+						done := make(chan struct{})
+						go func() {
+							defer close(done)
+
+							for _, onChange := range onChanges {
+								onChange(c)
+							}
+						}()
+
+						select {
+						case <-done:
+							slog.InfoContext(ctx, "Configuration has been applied to onChanges.")
+						case <-ctx.Done():
+							if errors.Is(ctx.Err(), context.DeadlineExceeded) {
+								slog.WarnContext(ctx, "Configuration has not been fully applied to onChanges due to timeout."+
+									" Please check if the onChanges is blocking or takes too long to complete.")
+							}
+						}
+					}()
 				}
 
 			case <-ctx.Done():
@@ -137,9 +173,9 @@
 
 	errChan := make(chan error, len(c.providers))
 	for _, provider := range c.providers {
-		if provider.watcher != nil {
-			provider := provider
+		provider := provider
 
+		if watcher, ok := provider.loader.(Watcher); ok {
 			waitGroup.Add(1)
 			go func() {
 				defer waitGroup.Done()
@@ -170,10 +206,12 @@
 
 					slog.Info(
 						"Configuration has been changed.",
-						"provider", provider.watcher,
+						"loader", watcher,
 					)
 				}
-				if err := provider.watcher.Watch(ctx, onChange); err != nil {
+
+				slog.Info("Watching configuration change.", "loader", watcher)
+				if err := watcher.Watch(ctx, onChange); err != nil {
 					errChan <- fmt.Errorf("watch configuration change: %w", err)
 					cancel()
 				}
@@ -218,8 +256,16 @@
 // It requires Config.Watch has been called first.
 // The paths are case-insensitive.
 //
+// The onChange function must be non-blocking and usually completes instantly.
+// If it requires a long time to complete, it should be executed in a separate goroutine.
+//
 // This method is concurrency-safe.
-func (c *Config) OnChange(onchange func(*Config), paths ...string) {
+// It panics if onChange is nil.
+func (c *Config) OnChange(onChange func(*Config), paths ...string) {
+	if onChange == nil {
+		panic("cannot register nil onChange")
+	}
+
 	c.onChangesMutex.Lock()
 	defer c.onChangesMutex.Unlock()
 
@@ -229,7 +275,7 @@
 
 	for _, path := range paths {
 		path = strings.ToLower(path)
-		c.onChanges[path] = append(c.onChanges[path], onchange)
+		c.onChanges[path] = append(c.onChanges[path], onChange)
 	}
 }
 

config_test.go

@@ -31,16 +31,6 @@ func TestConfig_Unmarshal(t *testing.T) {
 				assert.Equal(t, "", value)
 			},
 		},
-		{
-			description: "nil loader",
-			opts:        []konf.Option{},
-			loaders:     []konf.Loader{nil},
-			assert: func(config *konf.Config) {
-				var value string
-				assert.NoError(t, config.Unmarshal("config", &value))
-				assert.Equal(t, "", value)
-			},
-		},
 		{
 			description: "for primary type",
 			loaders:     []konf.Loader{mapLoader{"config": "string"}},

default.go

@@ -39,16 +39,26 @@
 // when the value of any given path in the default Config changes.
 // The paths are case-insensitive.
 //
+// The onChange function must be non-blocking and usually completes instantly.
+// If it requires a long time to complete, it should be executed in a separate goroutine.
+//
 // This method is concurrency-safe.
+// It panics if onChange is nil.
 func OnChange(onChange func(), paths ...string) {
 	defaultConfig.Load().OnChange(func(*Config) { onChange() }, paths...)
 }
 
 // SetDefault sets the given Config as the default Config.
 // After this call, the konf package's top functions (e.g. konf.Get)
 // will interact with the given Config.
-func SetDefault(c *Config) {
-	defaultConfig.Store(c)
+//
+// It panics if config is nil.
+func SetDefault(config *Config) {
+	if config == nil {
+		panic("cannot set default with nil config")
+	}
+
+	defaultConfig.Store(config)
 }
 
 var defaultConfig atomic.Pointer[Config] //nolint:gochecknoglobals

doc.go

@@ -18,6 +18,7 @@ Configuration is hierarchical, and the path is a sequence of keys that separated
 The default delimiter is `.`, which makes configuration path like `parent.child.key`.
 
 # Load Configuration
+
 After creating a [Config], you can load configuration from multiple [Loader](s) using [Config.Load].
 Each loader takes precedence over the loaders before it. As long as the configuration has been loaded,
 it can be used in following code to get or unmarshal configuration, even for loading configuration
@@ -28,7 +29,6 @@ and then use the file path to load configuration from file system.
 
 [Config.Watch] watches and updates configuration when it changes, which leads [Config.Unmarshal]
 always returns latest configuration.
-
 You may use [Config.OnChange] to register a callback if the value of any path have been changed.
 It could push the change into application objects instead pulling the configuration periodically.
 */

provider/env/env.go

@@ -29,12 +29,13 @@ type Env struct {
 
 // New creates an Env with the given Option(s).
 func New(opts ...Option) Env {
-	option := &options{
-		delimiter: "_",
-	}
+	option := &options{}
 	for _, opt := range opts {
 		opt(option)
 	}
+	if option.delimiter == "" {
+		option.delimiter = "_"
+	}
 
 	return Env(*option)
 }

provider/file/file.go

@@ -30,14 +30,22 @@
 }
 
 // New creates a File with the given path and Option(s).
+//
+// It panics if the path is empty.
 func New(path string, opts ...Option) File {
+	if path == "" {
+		panic("cannot create File with empty path")
+	}
+
 	option := &options{
-		path:      path,
-		unmarshal: json.Unmarshal,
+		path: path,
 	}
 	for _, opt := range opts {
 		opt(option)
 	}
+	if option.unmarshal == nil {
+		option.unmarshal = json.Unmarshal
+	}
 
 	return File(*option)
 }

provider/file/file_test.go

@@ -61,7 +61,7 @@ func TestFile_Load(t *testing.T) {
 			t.Parallel()
 
 			values, err := file.New(testcase.path, testcase.opts...).Load()
-			if err != nil {
+			if testcase.err != "" {
 				assert.True(t, strings.HasPrefix(err.Error(), testcase.err))
 			} else {
 				assert.NoError(t, err)

provider/flag/flag.go

@@ -32,12 +32,13 @@ type Flag struct {
 
 // New creates a Flag with the given Option(s).
 func New(opts ...Option) Flag {
-	option := &options{
-		delimiter: ".",
-	}
+	option := &options{}
 	for _, opt := range opts {
 		opt(option)
 	}
+	if option.delimiter == "" {
+		option.delimiter = "."
+	}
 	if option.set == nil {
 		flag.Parse()
 		option.set = flag.CommandLine

provider/fs/fs.go

@@ -32,15 +32,26 @@
 }
 
 // New creates a FS with the given fs.FS, path and Option(s).
-func New(fs fs.FS, path string, opts ...Option) FS {
+//
+// It panics if the fs is nil or the path is empty.
+func New(fs fs.FS, path string, opts ...Option) FS { //nolint:varnamelen
+	if fs == nil {
+		panic("cannot create FS with nil fs")
+	}
+	if path == "" {
+		panic("cannot create FS with empty path")
+	}
+
 	option := &options{
-		fs:        fs,
-		path:      path,
-		unmarshal: json.Unmarshal,
+		fs:   fs,
+		path: path,
 	}
 	for _, opt := range opts {
 		opt(option)
 	}
+	if option.unmarshal == nil {
+		option.unmarshal = json.Unmarshal
+	}
 
 	return FS(*option)
 }

provider/fs/fs_test.go

@@ -8,7 +8,6 @@ package fs_test
 import (
 	"errors"
 	"io/fs"
-	"strings"
 	"testing"
 	"testing/fstest"
 
@@ -80,8 +79,8 @@ func TestFile_Load(t *testing.T) {
 			t.Parallel()
 
 			values, err := kfs.New(testcase.fs, testcase.path, testcase.opts...).Load()
-			if err != nil {
-				assert.True(t, strings.HasPrefix(err.Error(), testcase.err))
+			if testcase.err != "" {
+				assert.EqualError(t, err, testcase.err)
 			} else {
 				assert.NoError(t, err)
 				assert.Equal(t, testcase.expected, values)

provider/pflag/pflag.go

@@ -34,12 +34,13 @@ type PFlag struct {
 
 // New creates a PFlag with the given Option(s).
 func New(opts ...Option) PFlag {
-	option := &options{
-		delimiter: ".",
-	}
+	option := &options{}
 	for _, opt := range opts {
 		opt(option)
 	}
+	if option.delimiter == "" {
+		option.delimiter = "."
+	}
 	if option.set == nil {
 		pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
 		pflag.Parse()