Skip to content
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

added ability to register custom log formatter #131

Merged
merged 8 commits into from
Jul 27, 2018
Merged

added ability to register custom log formatter #131

merged 8 commits into from
Jul 27, 2018

Conversation

mlsquires
Copy link
Contributor

PR in response to Issue 130:
#130

@kisielk
Copy link
Contributor

kisielk commented Jul 26, 2018

Can you change the function signature to accept a struct instead?
something like

Req *http.Request
URL url.URL
Timestamp time.Time
Status int
Size int
}

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.

@mlsquires
Copy link
Contributor Author

That makes a lot of sense. I'll do that now.

@mlsquires
Copy link
Contributor Author

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.

Copy link
Contributor

@kisielk kisielk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kisielk kisielk merged commit 7e0847f into gorilla:master Jul 27, 2018
@kisielk
Copy link
Contributor

kisielk commented Jul 27, 2018

Looks good! Thanks for the contribution!

@mlsquires mlsquires deleted the feature/customloggers branch July 29, 2018 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants