-
Notifications
You must be signed in to change notification settings - Fork 491
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
record: add logger to reader, update walSync test to use logger #4415
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions
record/record.go
line 257 at r1 (raw file):
// readerLogger is a logging helper used by the Reader to accumulate log messages. logger *readerLogger
Since this is for testing, the usual idiom is to have the field be called loggerForTesting
. And also hide the implementation in a test go file. Which implies declaring an interface here. So something like
loggerForTesting loggerForTesting
}
type loggerForTesting interface {
Logf(format string, args ...interface{})
}
record/record.go
line 264 at r1 (raw file):
} func (l *readerLogger) Log(s string) {
why do we need this method when we have Logf
which subsumes this behavior?
record/record.go
line 274 at r1 (raw file):
func (l *readerLogger) Logf(format string, args ...interface{}) { msg := fmt.Sprintf(format, args...)
fmt.Fprintf(&l.builder, format, args...)
should be sufficient.
705a5be
to
79ddb70
Compare
79ddb70
to
c433682
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @sumeerbhola)
record/record.go
line 257 at r1 (raw file):
Previously, sumeerbhola wrote…
Since this is for testing, the usual idiom is to have the field be called
loggerForTesting
. And also hide the implementation in a test go file. Which implies declaring an interface here. So something likeloggerForTesting loggerForTesting } type loggerForTesting interface { Logf(format string, args ...interface{}) }
done
record/record.go
line 264 at r1 (raw file):
Previously, sumeerbhola wrote…
why do we need this method when we have
Logf
which subsumes this behavior?
removed
record/record.go
line 274 at r1 (raw file):
Previously, sumeerbhola wrote…
fmt.Fprintf(&l.builder, format, args...)
should be sufficient.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r2.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @jbowens)
a discussion (no related file):
record/record.go
line 259 at r2 (raw file):
type loggerForTesting interface { Logf(format string, args ...interface{})
nit: logf (since package private is sufficient)
record/record.go
line 260 at r2 (raw file):
type loggerForTesting interface { Logf(format string, args ...interface{}) GetLog() string
getLog
doesn't need to be in the interface, since it is called by the test code which knows it is dealing with a readerLogger
.
add test logger to walSync test.
c433682
to
e37529b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @EdwardX29 and @jbowens)
a discussion (no related file):
add test logger to walSync test.