Add convenience methods Key() and Value() to log.Field #116
Conversation
RaduBerinde
force-pushed
the
field-stuff
branch
from
a315c71 to
372d82e
Compare
| // method. | ||
| // | ||
| // "heavily influenced by" (i.e., partially stolen from) | ||
| // https://github.com/uber-go/zap | ||
| type Field struct { | ||
| key string | ||
| Key string |
There was a problem hiding this comment.
would it make more sense to expose Key() instead? That would keep the Field immutable.
Currently the only way to print a `Field` is to define an `Encoder`. This is a
lot of code to write, especially if it's needed just for debugging or a quick
test. Adding a `Value()` method that returns the value as an `interface{}`
(which can be passed to `fmt.Print` functions), as well as a `String()` method.
Also exporting the key field.
RaduBerinde
force-pushed
the
field-stuff
branch
from
372d82e to
2250288
Compare
| return math.Float32frombits(uint32(lf.numericVal)) | ||
| case float64Type: | ||
| return math.Float64frombits(uint64(lf.numericVal)) | ||
| case errorType, objectType, lazyLoggerType: |
There was a problem hiding this comment.
I wonder what we should do for lazyLoggerType... I guess this is "fine for now", though I can imagine someone expecting to see the thing the lazy log function encodes instead.
There was a problem hiding this comment.
Ah.. Yeah I guess we would need to define an Encoder that captures the emitted key-value, and run the lazy logger. I'll file an issue.
BTW this makes me curious - why does the Encoder interface have an EmitLazyLogger method? Wouldn't any implementation of that (that follows the intended purpose) always just call the lazyLogger? Seems that instead of calling visitor.EmitLazyLogger(lf.interfaceVal.(LazyLogger)) we could call lf.interfaceVal.(LazyLogger)(visitor) no?
There was a problem hiding this comment.
IIRC this was adapted somewhat blindly from the zap approach... my only rationale would be that some impls might not want to do lazy logging at all, and the way things are structured now gives them more control (???). But I am just rationalizing.
There was a problem hiding this comment.
@RaduBerinde I think the Encoder serves two purposes. At the top level it allows the implementations to be told that a log field has a lazy encoder, so the respective method is needed. But then the same interface is reused recursively in the lazy encoder signature.
There was a problem hiding this comment.
I see, thanks. There are a few more things that could use some clarification, I filed #118 so we can move the discussion there.
| @@ -20,7 +23,7 @@ const ( | |||
| ) | |||
|
|
|||
| // Field instances are constructed via LogBool, LogString, and so on. | |||
| // Tracing implementations may then handle them via the Field.Process | |||
| // Tracing implementations may then handle them via the Field.Marshal | |||
yurishkuro
changed the title
Currently the only way to print a
Fieldis to define anEncoder. This is alot of code to write, especially if it's needed just for debugging or a quick
test. Adding a
Value()method that returns the value as aninterface{}(which can be passed to
fmt.Printfunctions), as well as aString()method.Also exporting the key field.
CC @tschottdorf