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

Use level-codec #313

Merged
merged 1 commit into from
Mar 24, 2015
Merged

Use level-codec #313

merged 1 commit into from
Mar 24, 2015

Conversation

juliangruber
Copy link
Member

This isn't yet ready to be merged, due to lacking sability in level-codec.

I initially factored out the encoding logic so I could use it in multilevel, but this also helps slim down levelup's code / responsibilities.

I'm thinking about moving more logic into level-codec later, in a separate pull request, like wrapping the callback for get / put and doing the encoding behind the scenes, instead of explicitly doing it in levelup.

What do you think?

As a bonus, I removed {encoding} which should be merged before 1.0.0 is published.

@juliangruber
Copy link
Member Author

oops, forgot to publish latest level-codec, see now: https://github.com/Level/codec/blob/master/index.js. The implementatino is a draft still with the purpose of having something that works so tests pass.

About passing in the codec: That neither was tested nor documented, so it looked like noone was using it.

@juliangruber
Copy link
Member Author

tests pass now

@mafintosh
Copy link
Member

does this mean i can't do this anymore?

db.get('foo', {valueEncoding: myOwnEncoder}, cb)

i usually do that when using protocol-buffers, https://github.com/mafintosh/protocol-buffers#leveldb-encoding-compatibility

@juliangruber
Copy link
Member Author

you totally can! in fact there's tests already in levelup to prove it. this doesn't change the external api at all, it's just an internal refactoring.

@mafintosh
Copy link
Member

@juliangruber 👍

@juliangruber
Copy link
Member Author

well, the only thing to change the internal api is the deprecation of .encoding, but there's a separate pull request to discuss this

@ralphtheninja
Copy link
Member

LGTM

@ralphtheninja
Copy link
Member

@juliangruber Mind squashing commits before merge? (Once enough people are OK with this change of course)

@juliangruber
Copy link
Member Author

I will, once level-codec is ready (read: works in multilevel as well, has stable api and more tests), and maybe waiting for more 👍s

@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 19, 2015

LGTM once you think level-codec is GTG @juliangruber. 👍 all the modularization.

@juliangruber
Copy link
Member Author

💃

@ralphtheninja ralphtheninja deleted the use/level-codec branch March 24, 2015 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants