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

Add KeyValueMap, a KeyValue implemented backed by a map #89

Merged
merged 1 commit into from
Jun 28, 2016

Conversation

prashantv
Copy link
Collaborator

This is a useful utility when testing custom MarhsalLog implementations

cc @billf: Can you make sure that this works for you to use in #85

This is a useful utility when testing custom MarhsalLog implementations
@akshayjshah
Copy link
Contributor

What do you think about putting this in its own package? Once we clean up and export the encoder type, we'll want this to implement the encoder interface, and we'll probably want to use it in some tests, in zwrap, and in zbark.

@prashantv
Copy link
Collaborator Author

Yep, I thought it belonged to another package, but wasn't sure what that package should be.

I considered zencoder but don't really think that's right. Any suggestions?

@akshayjshah
Copy link
Contributor

How about mapencoder? mapencoder.New sounds like a reasonable constructor.

@prashantv
Copy link
Collaborator Author

Hmm I'm not sure about encoder in the name since it's an implementation of the KeyValue interface. The interface abstracts the JSON encoder and encoding in general, but the KeyValueMap doesn't really fulfill a full encoder, it only handles the fields.

However, I don't have strong opinions on this so if we can't come up with anything better, then mapencoder is OK.

@akshayjshah
Copy link
Contributor

lgtm. let's drop it wherever makes sense, maybe in the core zap package.

@prashantv
Copy link
Collaborator Author

Leaving it in zwrap for now, we can move it in to core later as we make changes for 1.0

@prashantv prashantv merged commit fd612f8 into master Jun 28, 2016
@prashantv prashantv deleted the pv_keyvalue_map branch June 28, 2016 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants