-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
asBuffer fix #19
asBuffer fix #19
Conversation
…ding and custom encoding
Do we have tests for the |
@vweevers Added some tests. |
test/index.js
Outdated
} | ||
} | ||
|
||
encdown(down).iterator({ keyAsBuffer: false, valueAsBuffer: false }) |
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.
Can you write this in the same way as the get()
test? Because the keyAsBuffer
and valueAsBuffer
options should also be based on encoding.buffer
.
We used to have this code in levelup
:
options = this._codec.encodeLtgt(options)
options.keyAsBuffer = this._codec.keyAsBuffer(options)
options.valueAsBuffer = this._codec.valueAsBuffer(options)
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.
Sure!
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.
If you don't mind, I'll add an additional test and keep this one as it is.
Scratch that. Encodings should determine this, not options.
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.
Got me thinking. Big picture, if you use encoding-down
, then *asBuffer
is an internal property determined by encoding. But if you don't, then asBuffer
is a public option, like in this levelup
test. Perhaps that needs clarification in docs (eventually, as most people will use encoding-down
), but where?
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.
@vweevers Good point. The thought struck my mind as well. Make an issue for it for now so we can move out the discussion from this PR?
test/index.js
Outdated
var down = { | ||
iterator: function (options) { | ||
t.is(options.keyAsBuffer, true, '.keyAsBuffer defaults to true') | ||
t.is(options.valueAsBuffer, true, '.valueAsBuffer defaults to true') |
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 just noticed this is wrong since utf8
is default encoding and should produce buffer: false
@vweevers This should be better I think. Will squash afterwards. |
Closes #18