logr icon indicating copy to clipboard operation
logr copied to clipboard

funcr: Option to sync() on Error()

Open thockin opened this issue 3 years ago • 3 comments

To do this we'd need to change the API - function signature or something.

thockin avatar Jan 29 '22 08:01 thockin

Or need to add optional args to New, e.g.

diff --git a/funcr/funcr.go b/funcr/funcr.go
index e52f0cd..6238195 100644
--- a/funcr/funcr.go
+++ b/funcr/funcr.go
@@ -50,17 +50,36 @@ import (
 )
 
 // New returns a logr.Logger which is implemented by an arbitrary function.
-func New(fn func(prefix, args string), opts Options) logr.Logger {
-	return logr.New(newSink(fn, NewFormatter(opts)))
+// FIXME: comment
+func New(fn func(prefix, args string), opts Options, configs ...Config) logr.Logger {
+	sink := newSink(fn, NewFormatter(opts))
+	for _, cfg := range configs {
+		cfg(sink)
+	}
+	return logr.New(sink)
 }
 
 // NewJSON returns a logr.Logger which is implemented by an arbitrary function
 // and produces JSON output.
-func NewJSON(fn func(obj string), opts Options) logr.Logger {
+// FIXME: comment
+func NewJSON(fn func(obj string), opts Options, configs ...Config) logr.Logger {
 	fnWrapper := func(_, obj string) {
 		fn(obj)
 	}
-	return logr.New(newSink(fnWrapper, NewFormatterJSON(opts)))
+	sink := newSink(fnWrapper, NewFormatterJSON(opts))
+	for _, cfg := range configs {
+		cfg(sink)
+	}
+	return logr.New(sink)
+}
+
+// FIXME: comment
+type Config func(sink *fnlogger)
+
+func SetSync(fn func()) Config {
+	return func(sink *fnlogger) {
+		sink.sync = fn
+	}
 }
 
 // Underlier exposes access to the underlying logging function. Since
@@ -71,10 +90,11 @@ type Underlier interface {
 	GetUnderlying() func(prefix, args string)
 }
 
-func newSink(fn func(prefix, args string), formatter Formatter) logr.LogSink {
+func newSink(fn func(prefix, args string), formatter Formatter) *fnlogger {
 	l := &fnlogger{
 		Formatter: formatter,
 		write:     fn,
+		sync:      func() {},
 	}
 	// For skipping fnlogger.Info and fnlogger.Error.
 	l.Formatter.AddCallDepth(1)
@@ -156,6 +176,7 @@ const (
 type fnlogger struct {
 	Formatter
 	write func(prefix, args string)
+	sync  func()
 }
 
 func (l fnlogger) WithName(name string) logr.LogSink {
@@ -181,6 +202,7 @@ func (l fnlogger) Info(level int, msg string, kvList ...interface{}) {
 func (l fnlogger) Error(err error, msg string, kvList ...interface{}) {
 	prefix, args := l.FormatError(err, msg, kvList)
 	l.write(prefix, args)
+	l.sync()
 }
 
 func (l fnlogger) GetUnderlying() func(prefix, args string) {

thockin avatar Dec 03 '22 19:12 thockin

Adding parameters to New, even if they are optional, will still be considered an API break by apidiff because a function pointer of the old type won't work anymore - not a big deal, though.

Why can't the new function be a new field in Options?

pohly avatar Dec 04 '22 11:12 pohly

Options is used by both funcr.New and Formatter. It felt weird to put it there. Just idle playing for now.

On Sun, Dec 4, 2022, 3:25 AM Patrick Ohly @.***> wrote:

Adding parameters to New, even if they are optional, will still be considered an API break by apidiff because a function pointer of the old type won't work anymore - not a big deal, though.

Why can't the new function be a new field in Options?

— Reply to this email directly, view it on GitHub https://github.com/go-logr/logr/issues/137#issuecomment-1336386447, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKWAVFCQG72BNEHBJBKFKTWLR5S5ANCNFSM5NCTJKAA . You are receiving this because you were assigned.Message ID: @.***>

thockin avatar Dec 04 '22 16:12 thockin