Breaking out msg/time/level #33
Conversation
Fixes sirupsen#29 "time" should be a time.Time
|
Thanks for your PR! I'll try to take a look this week. |
| jsonEntry.Level = "fatal" | ||
| case Panic: | ||
| jsonEntry.Level = "panic" | ||
| } |
There was a problem hiding this comment.
What about making this a method on the Level?
There was a problem hiding this comment.
This one makes sense as a method on Level. The one for TextFormatter, not so much.
|
I like this a lot more. |
|
Can you check out the tests? |
| jsonEntry := jsonEntry{ | ||
| Time: entry.Time, | ||
| Msg: entry.Msg, | ||
| Data: entry.Data, |
There was a problem hiding this comment.
I'm not sure what I think about this as a default. Nested hashes aren't the greatest. I get where you're coming from though, since there's no way nice to have this work with keys with the same name.
|
bump |
|
An alternative is to serialize all the keys at the same level (not nested), but prefix all user supplied keys to avoid name collisions. This would solve the problem of fields being silently overwritten. |
|
@aybabtme not a fan of that, it breaks convenience of splunking etc. |
|
I'm opening #44 to discuss solutions to this problem. |
|
Closing in favor of #48. |
The first commit (fe381bf) just adds some text_formatter unit tests.
7679f44 changes
Entryto the structure I discuss in #29 and #30. It removesmsg,time, andlevelfrom theDatamap and makes them first-classEntryfields. This moves the mapping of level to string entirely into the formatters.This modifies the default JSON format, causing it to encapsulate additional fields in a
datasub-object. I believe this is a better JSON format because it doesn't have any magical fields. But it is trivial to implement the previous "flat" JSON in a formatter. Just copy the data map into a new map and merge-in the three magical fields, and marshal that. I'm happy to write aFlatJSONFormatterif you think it's useful to anyone.I also changed the JSON encoding of Warn to "warn" rather than "warning" for consistency. That's trivially reverted in json_formatter.
I've added
Unformat()methods on the formatters to assist in testing. I haven't added that as part of theFormatterinterface, however.Unformat()may be very inconvenient to write for some formats, and I don't know if it's worth making it mandatory. It's probably worth adding an explicitUnformatterinterface.I haven't finished the text unformatter yet; I only handle the colorized case. Testing the other case is a bit of a headache currently because of how it checks for the terminal. It is probably worth reworking ForceColor to an enum (color/plain/auto). The whole auto-detect has a lot of tricky corner cases, anyway. If the log output isn't stdout, then it's not clear what to check. Personally, I'd like color/non-color to be split into separate formatters and let the caller work out what they want.
I'd also recommend that the formatters be moved into sub-packages like the hooks. There's no reason to build all the formatters (and their dependencies, like the JSON encoder) when you're likely to only use one of them. I have a custom formatter package, and don't use any of these. I can make that change if it's acceptable.