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

funcr: Option to sync() on Error() #137

Open
thockin opened this issue Jan 29, 2022 · 3 comments
Open

funcr: Option to sync() on Error() #137

thockin opened this issue Jan 29, 2022 · 3 comments
Assignees

Comments

@thockin
Copy link
Contributor

thockin commented Jan 29, 2022

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

@thockin thockin self-assigned this Jan 29, 2022
@thockin
Copy link
Contributor Author

thockin commented Dec 3, 2022

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) {

@pohly
Copy link
Contributor

pohly commented Dec 4, 2022

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?

@thockin
Copy link
Contributor Author

thockin commented Dec 4, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants