added ability to register custom log formatter#131
added ability to register custom log formatter#131kisielk merged 8 commits intogorilla:masterfrom mlsquires:feature/customloggers
Conversation
|
Can you change the function signature to accept a struct instead? As I commented in the ticket, this will make the function signatures a bit cleaner and also future-proof it if more information is added to the logs at a later time. |
|
That makes a lot of sense. I'll do that now. |
… easier to see the scenarios
|
I had some suggestions for refactoring the handlers_test.go but was going to put them in another PR. But since we were switching the function signature to use a struct and I would have to re-work some of the tests as a result, I just added the test refactoring to this PR as well. BTW - I'm an experienced developer but have only been using Go for 4 months, so any suggestions about better / more idiomatic code are more than welcome. |
kisielk
left a comment
There was a problem hiding this comment.
Looks good apart from the one change.
Also since this handler and its tests are getting pretty big it's probably worthwhile to split it into its own files, something logging.go and logging_test.go
handlers.go
Outdated
|
|
||
| // FormatterParams is the structure any formatter will be handed when time to log comes | ||
| type FormatterParams struct { | ||
| Writer io.Writer |
There was a problem hiding this comment.
I didn't want to include the writer here, the goal wasn't to just put all the parameters into a struct but to separate "what is being logged" into just one field so that is extensible, and so similarly typed parameters like StatusCode and Size can't be accidentally transposed or misinterpreted. So I would put the writer back as a separate argument.
Also I'd prefix the name of this struct with LogFormatter because there are a lot of other handlers in this package apart from the logging handler, so it helps make it clear where it belongs.
|
Looks good! Thanks for the contribution! |
PR in response to Issue 130:
#130