Make optional parameter configuration saner

Author: lestrrat

interface.go

@@ -20,7 +20,7 @@ type RotateLogs struct {
 	outFh         *os.File
 	pattern       *strftime.Strftime
 	rotationTime  time.Duration
-	rotationCount int
+	rotationCount uint
 }
 
 // Clock is the interface used by the RotateLogs
@@ -41,9 +41,6 @@ var Local = clockFn(time.Now)
 // Option is used to pass optional arguments to
 // the RotateLogs constructor
 type Option interface {
-	Configure(*RotateLogs) error
+	Name() string
+	Value() interface {}
 }
-
-// OptionFn is a type of Option that is represented
-// by a single function that gets called for Configure()
-type OptionFn func(*RotateLogs) error

internal/option/option.go

@@ -0,0 +1,25 @@
+package option
+
+type Interface interface {
+	Name() string
+	Value() interface{}
+}
+
+type Option struct {
+	name  string
+	value interface{}
+}
+
+func New(name string, value interface{}) *Option {
+	return &Option{
+		name:  name,
+		value: value,
+	}
+}
+
+func (o *Option) Name() string {
+	return o.name
+}
+func (o *Option) Value() interface{} {
+	return o.value
+}

internal_test.go

@@ -39,13 +39,3 @@ func TestGenFilename(t *testing.T) {
 		}
 	}
 }
-
-func TestWithLocation(t *testing.T) {
-	// Not really a test, but well...
-	loc, _ := time.LoadLocation("Asia/Tokyo")
-	opt := WithLocation(loc)
-	var rl RotateLogs
-	opt.Configure(&rl)
-	t.Logf("%s", rl.clock.Now())
-}
-

options.go

@@ -0,0 +1,66 @@
+package rotatelogs
+
+import (
+	"time"
+
+	"github.com/lestrrat-go/file-rotatelogs/internal/option"
+)
+
+const (
+	optkeyClock         = "clock"
+	optkeyLinkName      = "link-name"
+	optkeyMaxAge        = "max-age"
+	optkeyRotationTime  = "rotation-time"
+	optkeyRotationCount = "rotation-count"
+)
+
+// WithClock creates a new Option that sets a clock
+// that the RotateLogs object will use to determine
+// the current time.
+//
+// By default rotatelogs.Local, which returns the
+// current time in the local time zone, is used. If you
+// would rather use UTC, use rotatelogs.UTC as the argument
+// to this option, and pass it to the constructor.
+func WithClock(c Clock) Option {
+	return option.New(optkeyClock, c)
+}
+
+// WithLocation creates a new Option that sets up a
+// "Clock" interface that the RotateLogs object will use
+// to determine the current time.
+//
+// This optin works by always returning the in the given
+// location.
+func WithLocation(loc *time.Location) Option {
+	return option.New(optkeyClock, clockFn(func() time.Time {
+		return time.Now().In(loc)
+	}))
+}
+
+// WithLinkName creates a new Option that sets the
+// symbolic link name that gets linked to the current
+// file name being used.
+func WithLinkName(s string) Option {
+	return option.New(optkeyLinkName, s)
+}
+
+// WithMaxAge creates a new Option that sets the
+// max age of a log file before it gets purged from
+// the file system.
+func WithMaxAge(d time.Duration) Option {
+	return option.New(optkeyMaxAge, d)
+}
+
+// WithRotationTime creates a new Option that sets the
+// time between rotation.
+func WithRotationTime(d time.Duration) Option {
+	return option.New(optkeyRotationTime, d)
+}
+
+// WithRotationCount creates a new Option that sets the
+// number of files should be kept before it gets
+// purged from the file system.
+func WithRotationCount(n uint) Option {
+	return option.New(optkeyRotationCount, n)
+}

rotatelogs.go

@@ -22,108 +22,64 @@ func (c clockFn) Now() time.Time {
 	return c()
 }
 
-func (o OptionFn) Configure(rl *RotateLogs) error {
-	return o(rl)
-}
-
-// WithClock creates a new Option that sets a clock
-// that the RotateLogs object will use to determine
-// the current time.
-//
-// By default rotatelogs.Local, which returns the
-// current time in the local time zone, is used. If you
-// would rather use UTC, use rotatelogs.UTC as the argument
-// to this option, and pass it to the constructor.
-func WithClock(c Clock) Option {
-	return OptionFn(func(rl *RotateLogs) error {
-		rl.clock = c
-		return nil
-	})
-}
-
-// WithLocation creates a new Option that sets up a
-// "Clock" interface that the RotateLogs object will use
-// to determine the current time.
-//
-// This optin works by always returning the in the given
-// location.
-func WithLocation(loc *time.Location) Option {
-	return WithClock(clockFn(func() time.Time {
-		return time.Now().In(loc)
-	}))
-}
-
-// WithLinkName creates a new Option that sets the
-// symbolic link name that gets linked to the current
-// file name being used.
-func WithLinkName(s string) Option {
-	return OptionFn(func(rl *RotateLogs) error {
-		rl.linkName = s
-		return nil
-	})
-}
-
-// WithMaxAge creates a new Option that sets the
-// max age of a log file before it gets purged from
-// the file system.
-func WithMaxAge(d time.Duration) Option {
-	return OptionFn(func(rl *RotateLogs) error {
-		if rl.rotationCount > 0 && d > 0 {
-			return errors.New("attempt to set MaxAge when RotationCount is also given")
-		}
-		rl.maxAge = d
-		return nil
-	})
-}
-
-// WithRotationTime creates a new Option that sets the
-// time between rotation.
-func WithRotationTime(d time.Duration) Option {
-	return OptionFn(func(rl *RotateLogs) error {
-		rl.rotationTime = d
-		return nil
-	})
-}
-
-// WithRotationCount creates a new Option that sets the
-// number of files should be kept before it gets
-// purged from the file system.
-func WithRotationCount(n int) Option {
-	return OptionFn(func(rl *RotateLogs) error {
-		if rl.maxAge > 0 && n > 0 {
-			return errors.New("attempt to set RotationCount when MaxAge is also given")
-		}
-		rl.rotationCount = n
-		return nil
-	})
-}
-
 // New creates a new RotateLogs object. A log filename pattern
 // must be passed. Optional `Option` parameters may be passed
-func New(pattern string, options ...Option) (*RotateLogs, error) {
-	globPattern := pattern
+func New(p string, options ...Option) (*RotateLogs, error) {
+	globPattern := p
 	for _, re := range patternConversionRegexps {
 		globPattern = re.ReplaceAllString(globPattern, "*")
 	}
 
-	strfobj, err := strftime.New(pattern)
+	pattern, err := strftime.New(p)
 	if err != nil {
 		return nil, errors.Wrap(err, `invalid strftime pattern`)
 	}
 
-	var rl RotateLogs
-	rl.clock = Local
-	rl.globPattern = globPattern
-	rl.pattern = strfobj
-	rl.rotationTime = 24 * time.Hour
-	// Keeping forward compatibility, maxAge is prior to rotationCount.
-	rl.maxAge = 7 * 24 * time.Hour
-	rl.rotationCount = -1
-	for _, opt := range options {
-		opt.Configure(&rl)
+	var clock Clock = Local
+	rotationTime := 24 * time.Hour
+	var rotationCount uint
+	var linkName string
+	var maxAge time.Duration
+
+	for _, o := range options {
+		switch o.Name() {
+		case optkeyClock:
+			clock = o.Value().(Clock)
+		case optkeyLinkName:
+			linkName = o.Value().(string)
+		case optkeyMaxAge:
+			maxAge = o.Value().(time.Duration)
+			if maxAge < 0 {
+				maxAge = 0
+			}
+		case optkeyRotationTime:
+			rotationTime = o.Value().(time.Duration)
+			if rotationTime < 0 {
+				rotationTime = 0
+			}
+		case optkeyRotationCount:
+			rotationCount = o.Value().(uint)
+		}
+	}
+
+	if maxAge > 0 && rotationCount > 0 {
+		return nil, errors.New("options MaxAge and RotationCount cannot be both set")
+	}
+
+	if maxAge == 0 && rotationCount == 0 {
+		// if both are 0, give maxAge a sane default
+		maxAge = 7 * 24 * time.Hour
 	}
 
-	return &rl, nil
+	return &RotateLogs{
+		clock:         clock,
+		globPattern:   globPattern,
+		linkName:      linkName,
+		maxAge:        maxAge,
+		pattern:       pattern,
+		rotationTime:  rotationTime,
+		rotationCount: rotationCount,
+	}, nil
 }
 
 func (rl *RotateLogs) genFilename() string {
@@ -171,7 +127,7 @@ func (rl *RotateLogs) getWriter_nolock(bailOnRotateFail bool) (io.Writer, error)
 		if bailOnRotateFail {
 			// Failure to rotate is a problem, but it's really not a great
 			// idea to stop your application just because you couldn't rename
-			// your log. 
+			// your log.
 			// We only return this error when explicitly needed.
 			return nil, err
 		}
@@ -288,11 +244,11 @@ func (rl *RotateLogs) rotate_nolock(filename string) error {
 
 	if rl.rotationCount > 0 {
 		// Only delete if we have more than rotationCount
-		if rl.rotationCount >= len(toUnlink) {
+		if rl.rotationCount >= uint(len(toUnlink)) {
 			return nil
 		}
 
-		toUnlink = toUnlink[:len(toUnlink)-rl.rotationCount]
+		toUnlink = toUnlink[:len(toUnlink)-int(rl.rotationCount)]
 	}
 
 	if len(toUnlink) <= 0 {

rotatelogs_test.go

@@ -153,8 +153,8 @@ func TestLogRotationCount(t *testing.T) {
 		rl, err := rotatelogs.New(
 			filepath.Join(dir, "log%Y%m%d%H%M%S"),
 			rotatelogs.WithClock(clock),
-			rotatelogs.WithMaxAge(-1),
-			rotatelogs.WithRotationCount(-1),
+			rotatelogs.WithMaxAge(time.Duration(0)),
+			rotatelogs.WithRotationCount(0),
 		)
 		if !assert.NoError(t, err, `Both of maxAge and rotationCount is disabled`) {
 			return
@@ -169,10 +169,12 @@ func TestLogRotationCount(t *testing.T) {
 			rotatelogs.WithMaxAge(1),
 			rotatelogs.WithRotationCount(1),
 		)
-		if !assert.NoError(t, err, `Both of maxAge and rotationCount is enabled`) {
+		if !assert.Error(t, err, `Both of maxAge and rotationCount is enabled`) {
 			return
 		}
-		defer rl.Close()
+		if rl != nil {
+			defer rl.Close()
+		}
 	})
 
 	t.Run("Only latest log file is kept", func(t *testing.T) {
@@ -272,8 +274,8 @@ func TestGHIssue16(t *testing.T) {
 	}()
 
 	dir, err := ioutil.TempDir("", "file-rotatelogs-gh16")
-	if err != nil {
-		t.Errorf("Failed to create temporary directory: %s", err)
+	if !assert.NoError(t, err, `creating temporary directory should succeed`) {
+		return
 	}
 	defer os.RemoveAll(dir)