Offer the ability to pass through standard capnslog when embedding. #53
Conversation
|
Here is one patch I've made to work around etcd-io/etcd#4115 It doesn't affect any of the capnslog functionality anywhere else as it's only an additional formatter which nobody has to use. I think it could be useful for embedding etcd. It's a bit of a hack, but it helps for now. You can use it with: |
|
FWIW I added a second patch with an even more elegant formatter for use in embedding etcd. Cheers |
| @@ -28,6 +29,28 @@ type Formatter interface { | |||
| Flush() | |||
| } | |||
|
|
|||
| func NewSimpleFormatter(w io.Writer) *SimpleFormatter { | |||
| return &SimpleFormatter{ | |||
| w: bufio.NewWriter(w), | |||
There was a problem hiding this comment.
It's unused in the lib, but useful to someone consuming it.
There was a problem hiding this comment.
It's not exported, so no-one can consume it :-)
There was a problem hiding this comment.
Why isn't it exported??
There was a problem hiding this comment.
In Go, a name is exported if it begins with a capital letter.
When importing a package, you can refer only to its exported names. Any "unexported" names are not accessible from outside the package.
There was a problem hiding this comment.
This I know, but either I misunderstand where the git comments are pointing to, or I don't see the lack of capitalization... Are we talking about: NewSimpleFormatter ? It looks capitalized to me.
There was a problem hiding this comment.
Talking about the field w on the SimpleFormatter struct - e.g. this won't work:
import "cpnslog"
f := capnslog.NewSimpleFormatter(bytes.Buffer{})
fmt.Println(f.w) // can't access w from outside package!
There was a problem hiding this comment.
and even if it IS exported I don't see what it provides to consumers..
purpleidea
force-pushed
the
feat/simple-formatter
branch
from
675a83a to
9aa8a60
Compare
|
@jonboulle I believe I fixed all the issues, rebased and pushed them back. Annoying that GH looses context on old comments when you force push :P |
| } | ||
|
|
||
| // Format builds a log message for the LogFormatter. The LogLevel is ignored. | ||
| func (s *LogFormatter) Format(pkg string, l LogLevel, i int, entries ...interface{}) { |
There was a problem hiding this comment.
nit, but can you change the receiver name to "l" or "f" or "lf" instead of "s"? here and for Flush()
There was a problem hiding this comment.
Also consider (pkg string, _ logLevel, i int, entries ...interface{})
|
Few final comments. Could you please also tweak the first line of your commit message? or similar. Thanks! |
This adds a more clever formatter that uses the logger call depth to return accurate data about the logged message in question. It can be called with: capnslog.SetFormatter( capnslog.NewLogFormatter(os.Stderr, "<prefix> ", flags) )
purpleidea
force-pushed
the
feat/simple-formatter
branch
from
e5093b5 to
68c9954
Compare
|
@jonboulle This was actually a really excellent review (even the nitpick) thanks! Can I get you as a reviewer on https://github.com/purpleidea/mgmt/ ? I've rebased everything, and also fixed the StringFormatter return value in a separate patch. LMK. |
|
@purpleidea you can try CCing me on some reviews, but I can't promise I'll get to anything in a timely manner :-) (note the two week gap on this one). Thanks for your patience - this LGTM. |
|
woo! Mgmt will get more interesting as the etcd parts get more cleaned up and merged :) Cheers |
This resolves: etcd-io/etcd#4115