-
Notifications
You must be signed in to change notification settings - Fork 2k
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
New encoding layer #986
New encoding layer #986
Conversation
type DecoderRegistry struct { | ||
decoders map[string]Decoder | ||
|
||
mu sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if matters, but can use sync.Map to avoid explicitly synchronize access to the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yeah, interesting idea. For now, I think type safety is more important than performance, especially since this mutex won't be locked more than 5-10 times during an application's lifecycle. But I will certainly play with it in the next round of changes (this is not the final form of the encoding package).
Thanks!
|
||
// RegisterDecoder registers a Decoder for a format. | ||
// Registering a Decoder for an already existing format is not supported. | ||
func (e *DecoderRegistry) RegisterDecoder(format string, enc Decoder) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would defining type Key string
and replacing format string
with format Key
both here and in the Decode()
method be appropriate?
It's a trivial change, but cognitively it highlights the relationship between the parameters across the different methods while still allowing consumers to use strings.
Ditto for the encoder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it'd be. What does Key
mean? Format I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps Key
is the wrong type name - perhaps type Format string
is better. The crux of the suggestion is to just better highlight the relationship across methods rather than relying on variable names; perhaps it's unnecessary though.
Signed-off-by: Mark Sagi-Kazar <[email protected]>
Signed-off-by: Mark Sagi-Kazar <[email protected]>
Signed-off-by: Mark Sagi-Kazar <[email protected]>
Signed-off-by: Mark Sagi-Kazar <[email protected]>
I'm going to merge this PR for now. I've created a note in the Viper 2 board with the next steps (will probably convert it to an issue). |
This PR adds an encoding layer to Viper, separating direct calls to encoding libraries to different packages and ultimately moving out most of the logic from the main Viper package.
The package implements the proposal from #888 with the exception of moving them out from the module to preserve backwards compatibility. That cannot happen in the v1 branch as it would be a breaking change, but introducing an encoding layer this way allows us to move things out of the module easily in the future.
Although this is a sane proposal and I believe it's the way forward, the abstraction is not perfect and we can already see some shortcomings:
map[string]interface{}
well (compared to JSON and YAML anyway), so it contains some conditional logic specific to that type.All this points towards a much higher level abstraction where we pass and return
map[string]interface{}
directly. The current implementation is a low level interface based on the Marshal/Unmarshal style ofencoding/json
.Something like this would probably be better:
There are also some other missing pieces from this PR:
I'm not sure whether this change should make its way to Viper as it is. On one hand it would be nice to start experimenting, on the other hand this is not a perfect solution and will require some changes for the long-term. I'll leave this PR here for now, wait for some potential feedback and merge it if it still feels right after a few days/weeks.