Refactor DefaultSinks() into RegisterSink

Akin to RegisterEncoder

config.go

@@ -21,8 +21,6 @@
 package zap
 
 import (
-	"io"
-	"os"
 	"sort"
 	"time"
 
@@ -166,20 +164,12 @@ func NewDevelopmentConfig() Config {
 
 // Build constructs a logger from the Config and Options.
 func (cfg Config) Build(opts ...Option) (*Logger, error) {
-	return cfg.BuildWithSinks(DefaultSinks(), opts...)
-}
-
-// BuildWithSinks uses the map of sinks to construct a logger from the
-// passed options. For example, the option "stdout" will construct a logger
-// that routes output to the stdout sink, but "/filepath" will fallback
-// to writing to file.
-func (cfg Config) BuildWithSinks(sm map[string]Sink, opts ...Option) (*Logger, error) {
 	enc, err := cfg.buildEncoder()
 	if err != nil {
 		return nil, err
 	}
 
-	sink, errSink, err := cfg.openSinks(sm)
+	sink, errSink, err := cfg.openSinks()
 	if err != nil {
 		return nil, err
 	}
@@ -235,12 +225,12 @@ func (cfg Config) buildOptions(errSink zapcore.WriteSyncer) []Option {
 	return opts
 }
 
-func (cfg Config) openSinks(sm map[string]Sink) (zapcore.WriteSyncer, zapcore.WriteSyncer, error) {
-	sink, closeOut, err := OpenWithSinks(sm, cfg.OutputPaths...)
+func (cfg Config) openSinks() (zapcore.WriteSyncer, zapcore.WriteSyncer, error) {
+	sink, closeOut, err := Open(cfg.OutputPaths...)
 	if err != nil {
 		return nil, nil, err
 	}
-	errSink, _, err := OpenWithSinks(sm, cfg.ErrorOutputPaths...)
+	errSink, _, err := Open(cfg.ErrorOutputPaths...)
 	if err != nil {
 		closeOut()
 		return nil, nil, err
@@ -251,24 +241,3 @@ func (cfg Config) openSinks(sm map[string]Sink) (zapcore.WriteSyncer, zapcore.Wr
 func (cfg Config) buildEncoder() (zapcore.Encoder, error) {
 	return newEncoder(cfg.Encoding, cfg.EncoderConfig)
 }
-
-// Sink defines the interface to write to and close logger destinations.
-type Sink interface {
-	zapcore.WriteSyncer
-	io.Closer
-}
-
-// DefaultSinks generates a map of regularly used writeSyncers, coupled
-// with the appropriate Closer.
-func DefaultSinks() map[string]Sink {
-	return map[string]Sink{
-		"stdout": NopCloserSink{os.Stdout},
-		"stderr": NopCloserSink{os.Stderr},
-	}
-}
-
-// NopCloserSink wraps a WriteSyncer with a no-op Close() method.
-type NopCloserSink struct{ zapcore.WriteSyncer }
-
-// Close does nothing (no-op).
-func (NopCloserSink) Close() error { return nil }

sink.go

@@ -0,0 +1,59 @@
+package zap
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"sync"
+
+	"go.uber.org/zap/zapcore"
+)
+
+var (
+	_sinkMutex      sync.RWMutex
+	errSinkNotFound = errors.New("sink for the given key not found")
+	_sinkFactories  = map[string]func() (Sink, error){
+		"stdout": func() (Sink, error) { return NopCloserSink{os.Stdout}, nil },
+		"stderr": func() (Sink, error) { return NopCloserSink{os.Stderr}, nil },
+	}
+)
+
+// Sink defines the interface to write to and close logger destinations.
+type Sink interface {
+	zapcore.WriteSyncer
+	io.Closer
+}
+
+// RegisterSink adds a Sink at the given key so it can be referenced
+// in config OutputPaths.
+func RegisterSink(key string, sinkFactory func() (Sink, error)) error {
+	_sinkMutex.Lock()
+	defer _sinkMutex.Unlock()
+	if key == "" {
+		return errors.New("sink key cannot be blank")
+	}
+	if _, ok := _sinkFactories[key]; ok {
+		return fmt.Errorf("sink already registered for key %q", key)
+	}
+	_sinkFactories[key] = sinkFactory
+	return nil
+}
+
+// newSink invokes the registered sink factory to create and return the
+// sink for the given key. Returns errSinkNotFound if the key cannot be found.
+func newSink(key string) (Sink, error) {
+	_sinkMutex.RLock()
+	defer _sinkMutex.RUnlock()
+	sinkFactory, ok := _sinkFactories[key]
+	if !ok {
+		return nil, errSinkNotFound
+	}
+	return sinkFactory()
+}
+
+// NopCloserSink wraps a WriteSyncer with a no-op Close() method.
+type NopCloserSink struct{ zapcore.WriteSyncer }
+
+// Close does nothing (no-op).
+func (NopCloserSink) Close() error { return nil }

sink_test.go

@@ -0,0 +1,55 @@
+package zap
+
+import (
+	"errors"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRegisterSink(t *testing.T) {
+	tests := []struct {
+		name      string
+		key       string
+		factory   func() (Sink, error)
+		wantError bool
+	}{
+		{"valid", "valid", func() (Sink, error) { return NopCloserSink{os.Stdout}, nil }, false},
+		{"empty", "", func() (Sink, error) { return NopCloserSink{os.Stdout}, nil }, true},
+		{"stdout", "stdout", func() (Sink, error) { return NopCloserSink{os.Stdout}, nil }, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := RegisterSink(tt.key, tt.factory)
+			if tt.wantError {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.NotNil(t, _sinkFactories[tt.key], "expected the factory to be present")
+			}
+		})
+	}
+}
+
+func TestNewSink(t *testing.T) {
+	errTestSink := errors.New("test erroring")
+	err := RegisterSink("errors", func() (Sink, error) { return nil, errTestSink })
+	assert.Nil(t, err)
+	tests := []struct {
+		key string
+		err error
+	}{
+		{"stdout", nil},
+		{"errors", errTestSink},
+		{"nonexistent", errSinkNotFound},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.key, func(t *testing.T) {
+			_, err := newSink(tt.key)
+			assert.Equal(t, tt.err, err)
+		})
+	}
+}

writer.go

@@ -38,20 +38,7 @@ import (
 // Passing no paths returns a no-op WriteSyncer. The special paths "stdout" and
 // "stderr" are interpreted as os.Stdout and os.Stderr, respectively.
 func Open(paths ...string) (zapcore.WriteSyncer, func(), error) {
-	return OpenWithSinks(DefaultSinks(), paths...)
-}
-
-// OpenWithSinks is a high-level wrapper that takes a map of sinks and a
-// variadic number of paths. The map customizes how to open and close a
-// particular path, with the default being the creation of a file
-// at said path. e.g. "stdout", "/file/destination".
-// It then combines all of the writers into a locked WriteSyncer and
-// returns any error encountered and a function to close any opened files.
-//
-// Passing no paths returns a no-op WriteSyncer. The special paths "stdout" and
-// "stderr" are interpreted as os.Stdout and os.Stderr, respectively.
-func OpenWithSinks(sm map[string]Sink, paths ...string) (zapcore.WriteSyncer, func(), error) {
-	writers, close, err := open(sm, paths)
+	writers, close, err := open(paths)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -60,7 +47,7 @@ func OpenWithSinks(sm map[string]Sink, paths ...string) (zapcore.WriteSyncer, fu
 	return writer, close, nil
 }
 
-func open(sm map[string]Sink, paths []string) ([]zapcore.WriteSyncer, func(), error) {
+func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
 	var openErr error
 	writers := make([]zapcore.WriteSyncer, 0, len(paths))
 	closers := make([]io.Closer, 0, len(paths))
@@ -70,16 +57,19 @@ func open(sm map[string]Sink, paths []string) ([]zapcore.WriteSyncer, func(), er
 		}
 	}
 	for _, path := range paths {
-		if sink, ok := sm[path]; ok {
+		if sink, err := newSink(path); err == nil {
 			writers = append(writers, sink)
 			closers = append(closers, sink)
 			continue
-		}
-		f, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
-		openErr = multierr.Append(openErr, err)
-		if err == nil {
-			writers = append(writers, f)
-			closers = append(closers, f)
+		} else if err == errSinkNotFound {
+			f, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
+			openErr = multierr.Append(openErr, err)
+			if err == nil {
+				writers = append(writers, f)
+				closers = append(closers, f)
+			}
+		} else {
+			openErr = multierr.Append(openErr, err)
 		}
 	}
 

writer_test.go

@@ -114,10 +114,11 @@ func (w *testWriter) Sync() error {
 	return nil
 }
 
-func TestOpenWithSinksCustomSink(t *testing.T) {
+func TestOpenWithCustomSink(t *testing.T) {
 	tw := &testWriter{"test", t}
-	sinks := map[string]Sink{"customsink": NopCloserSink{tw}}
-	w, cleanup, err := OpenWithSinks(sinks, "customsink")
+	ctr := func() (Sink, error) { return NopCloserSink{tw}, nil }
+	assert.Nil(t, RegisterSink("customsink", ctr))
+	w, cleanup, err := Open("customsink")
 	assert.Nil(t, err)
 	defer cleanup()
 	w.Write([]byte("test"))