Add RegisterEncoder functionality #348
Conversation
|
@peter-edge, thanks for your PR! By analyzing the history of the files in this pull request, we identified @akshayjshah and @suyash to be potential reviewers. |
encoder.go
Outdated
| return errNoEncoderNameSpecified | ||
| } | ||
| if _, ok := encoderNameToConstructor[name]; ok { | ||
| return fmt.Errorf("encoder already registered for name %s", name) |
There was a problem hiding this comment.
%q may be a moderately more useful formatting code here
encoder.go
Outdated
| if !ok { | ||
| return nil, fmt.Errorf("no encoder registered for name %s", name) | ||
| } | ||
| return constructor(encoderConfig), nil |
There was a problem hiding this comment.
Seems a missed opportunity to not let the encoder constructor return an error (e.g. config validation if needed).
There was a problem hiding this comment.
Ya I was thinking the same thing, didn't know what to do. I updated to this.
encoder_test.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
is the var binding truly needed?
|
All comments addressed |
There was a problem hiding this comment.
This looks okay to me, but I bet @akshayjshah will really want those _ prefixes on your globals ;-)
At any rate, leaving actual stamp bit for him.
|
Ya I bet he will too, let's hope he doesn't notice :) Fat chance :) |
encoder.go
Outdated
| var ( | ||
| errNoEncoderNameSpecified = errors.New("no encoder name specified ") | ||
| encoderNameToConstructor = make(map[string]func(zapcore.EncoderConfig) (zapcore.Encoder, error)) | ||
| encoderMutex sync.RWMutex |
There was a problem hiding this comment.
nit: please underscore-prefix globals (except for errors).
I'm not wild about this convention either, but let's keep everything consistent.
encoder.go
Outdated
| if err := RegisterEncoder(name, constructor); err != nil { | ||
| panic(err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Peter, I know you're not wild about this Must* pattern; in this case, I'm not a fan either. Can we drop this and encourage users to handle errors properly?
encoder.go
Outdated
| return func(encoderConfig zapcore.EncoderConfig) (zapcore.Encoder, error) { | ||
| return constructor(encoderConfig), nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we simplify a bit by declaring encoderNameToConstructor as a map literal (or the output of a defaultEncoders() function)? I think that'll let us drop this wrapper function, MustRegisterEncoder, registerDefaultEncoders, and init.
(This is actually a question, not a requirement - I'm fine with the current approach too.)
|
Haha, didn't see the discussion about underscores 😄 |
|
All comments addressed |
|
Last nit: please add messages in assertions. |
encoder.go
Outdated
| return constructor(encoderConfig) | ||
| } | ||
|
|
||
| func defaultEncoders() map[string]func(zapcore.EncoderConfig) (zapcore.Encoder, error) { |
There was a problem hiding this comment.
Do we even need this function now?
|
Both comments should be addressed |
|
@akshayjshah note you have to merge this :) |
For #330
This is just a pass at this, let me know of any changes. I purposefully didn't do the underscores in front of private variables (I don't want to do this anymore) but I can add this for consistency. We could reuse the global mutex in global.go as well.