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

New encoding layer #986

Merged
merged 6 commits into from
Jun 25, 2021
Merged

New encoding layer #986

merged 6 commits into from
Jun 25, 2021

Conversation

sagikazarmark
Copy link
Collaborator

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:

  • TOML does not support map[string]interface{} well (compared to JSON and YAML anyway), so it contains some conditional logic specific to that type.
  • Many supported formats (INI, dotenv, Java properties) don't have a JSON/YAML style "unmarshal" API. In fact, some of them are completely different:
    • In case of dotenv, the env var names are generated by formatting the keys
    • In case of INI sections are created from keys

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 of encoding/json.

Something like this would probably be better:

// Decoder decodes the contents of b into a map[string]interface{}.
type Decoder interface {
	Decode(b []byte) (map[string]interface{}, error)
}

// Encoder encodes the contents of v into a raw byte representation.
type Encoder interface {
	Encode(v map[string]interface{}) ([]byte, error)
}

There are also some other missing pieces from this PR:

  • Proper tests for the different codecs (currently we rely on Viper's tests which are...not necessarily good)
  • Some codecs might have additional config options

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.

@sagikazarmark sagikazarmark added this to In progress in Viper 2 via automation Sep 23, 2020
@sagikazarmark sagikazarmark added the kind/enhancement New feature or request label Sep 23, 2020
type DecoderRegistry struct {
decoders map[string]Decoder

mu sync.RWMutex

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.

Copy link
Collaborator Author

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

@chrisdoherty4 chrisdoherty4 Dec 2, 2020

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.

Copy link
Collaborator Author

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.

Copy link

@chrisdoherty4 chrisdoherty4 Dec 3, 2020

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 <mark.sagikazar@gmail.com>
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark
Copy link
Collaborator Author

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).

@sagikazarmark sagikazarmark merged commit a02f986 into master Jun 25, 2021
Viper 2 automation moved this from In progress to Done Jun 25, 2021
@sagikazarmark sagikazarmark deleted the encoding branch June 25, 2021 12:26
@sagikazarmark sagikazarmark mentioned this pull request Jul 16, 2021
4 tasks
@sagikazarmark sagikazarmark added this to the v2 milestone Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
Viper 2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants