registry: Allow configuring the logger #521
Conversation
|
cc @clrprod |
Codecov Report
@@ Coverage Diff @@
## master #521 +/- ##
==========================================
+ Coverage 73.13% 73.15% +0.01%
==========================================
Files 92 92
Lines 3905 3915 +10
==========================================
+ Hits 2856 2864 +8
- Misses 690 691 +1
- Partials 359 360 +1
Continue to review full report at Codecov.
|
|
Sorry for the delay taking a look now. |
| // Log is used to log requests. | ||
| // If nil, the global logger is used. | ||
| Log *log.Logger | ||
| } |
There was a problem hiding this comment.
Looks awesome - I think this is definitely a pretty reasonable thing to change and thank you for sending in the change.
They style in go is generally to define a option type - i.e.
type Option func(o *registry)
And then have New take variadic arguments - i.e.
New(o Option...)
and in the body do something like
opts := ®istry{log:defaultLog}
for _, o := range options {
o(opts)
}
And define the logger option like
func WithLogger(l log.Type) Option
so you can call New() to get default behaviour, New(WithLogger(log.SomethingComplicated)) to get more interesting behaviour.
Let me know if that makes sense - I'm happy to have the log be configurable, just want to make sure the option pattern is nice for the user. I'm also happy to make these changes if I'm asking for too much work from you.
There was a problem hiding this comment.
They style in go is generally to define a option type - i.e.
Its another pattern for sure but I wouldn't say its generally better. The stdlib for example doesn't use it at all and even though later packages did, the new go-cloud package made by the Go team explicitly decided against using them in favour of optional structs.
There was a problem hiding this comment.
I'm also happy to make these changes if I'm asking for too much work from you.
Feel free to take over.
There was a problem hiding this comment.
I think both approaches are reasonable. There are definitely some hard to solve problems with the functional options approach, but it looks so much nicer in some cases 😄
We should keep in mind any other extension points e.g. #488
There was a problem hiding this comment.
I pushed a version of this in #537 - let me know what you think :D
|
Closed in favour of #537 |
No description provided.