Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

writer: add CombineWriteSyncers #346

Merged
merged 4 commits into from
Mar 8, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,13 @@ 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) {
if len(paths) == 0 {
return zapcore.AddSync(ioutil.Discard), func() {}, nil
}

writers, close, err := open(paths)
if len(writers) == 1 {
return zapcore.Lock(writers[0]), close, err
if err != nil {
return nil, nil, err
}
return zapcore.Lock(zapcore.NewMultiWriteSyncer(writers...)), close, err

writer := CombineWriteSyncers(writers...)
return writer, close, nil
}

func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
Expand Down Expand Up @@ -76,3 +74,12 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
}
return writers, close, errs.AsError()
}

// CombineWriteSyncers combines the passed set of WriteSyncer objects into a
// locked WriteSyncer.
func CombineWriteSyncers(writers ...zapcore.WriteSyncer) zapcore.WriteSyncer {
if len(writers) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might just return nil here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to return a discarder, purely for consistency with New.

return zapcore.AddSync(ioutil.Discard)
}
return zapcore.Lock(zapcore.NewMultiWriteSyncer(writers...))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this function; I'd much rather export just Open.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshayjshah the whole point of this PR was to get a function in the API that takes interfaces instead of strings as output file paths.

Please see my first comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, my fault - I confused this with the previously-present OpenWriters.

Since the files/writers are already open, let's rename this to CombineWriteSyncers or ComposeWriteSyncers instead. This function can't fail, so let's also remove the error return value.

20 changes: 20 additions & 0 deletions writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,23 @@ func TestOpen(t *testing.T) {
assert.Equal(t, tt.filenames, names, "Opened unexpected files given paths %v.", tt.paths)
}
}

type testWriter struct {
expected string
t testing.TB
}

func (w *testWriter) Write(actual []byte) (int, error) {
assert.Equal(w.t, []byte(w.expected), actual, "Unexpected write error.")
return len(actual), nil
}

func (w *testWriter) Sync() error {
return nil
}

func TestCombineWriteSyncers(t *testing.T) {
tw := &testWriter{"test", t}
w := CombineWriteSyncers(tw)
w.Write([]byte("test"))
}
3 changes: 3 additions & 0 deletions zapcore/write_syncer.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ type multiWriteSyncer []WriteSyncer
// NewMultiWriteSyncer creates a WriteSyncer that duplicates its writes
// and sync calls, much like io.MultiWriter.
func NewMultiWriteSyncer(ws ...WriteSyncer) WriteSyncer {
if len(ws) == 1 {
return ws[0]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, Currently simply verifying that passing just one argument to NewMultiWriteSyncer simply returns the argument back.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Excellent.

// Copy to protect against https://github.com/golang/go/issues/7809
return multiWriteSyncer(append([]WriteSyncer(nil), ws...))
}
Expand Down
10 changes: 10 additions & 0 deletions zapcore/write_syncer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ func TestAddSyncWriter(t *testing.T) {
assert.NoError(t, ws.Sync(), "Unexpected error calling a no-op Sync method.")
}

func TestNewMultiWriteSyncerWorksForSingleWriter(t *testing.T) {
w := &testutils.Buffer{}

ws := NewMultiWriteSyncer(w)
assert.Equal(t, w, ws, "Expected NewMultiWriteSyncer to return the same WriteSyncer object for a single argument.")

ws.Sync()
assert.True(t, w.Called(), "Expected Sync to be called on the created WriteSyncer")
}

func TestMultiWriteSyncerWritesBoth(t *testing.T) {
first := &bytes.Buffer{}
second := &bytes.Buffer{}
Expand Down