search

Level / abstract-leveldown

An abstract prototype matching the leveldown API.
MIT License
146 stars 53 forks source link

Default _seek is incorrect? #301

Closed ralphtheninja closed 6 years ago

ralphtheninja commented 6 years ago

.seek(target) calls ._seek(target)

https://github.com/Level/abstract-leveldown/blob/aeabe9951e13cfcfa1454a0fec2691bc6684ea08/abstract-iterator.js#L37-L47

But default implementation treats target as a callback.

https://github.com/Level/abstract-leveldown/blob/aeabe9951e13cfcfa1454a0fec2691bc6684ea08/abstract-iterator.js#L49-L51

ralphtheninja commented 6 years ago

@vweevers I'm guessing it should just be a noop function?

vweevers commented 6 years ago

Jeez, good catch!

vweevers commented 6 years ago

btw I have a leveldown branch ready but had no chance to create a PR yet (to next). maybe tomorrow

ralphtheninja commented 6 years ago

Jeez, good catch!

Found it while trying to write tests for full coverage :) nyc and coveralls rock!

vweevers commented 6 years ago

Importing tests from leveldown will get us coverage of seek

ralphtheninja commented 6 years ago

Importing tests from leveldown will get us coverage of seek

I'm a bit unsure of exactly which tests to import from leveldown and where to put them. Should all of the extra tests here be imported?

Afaict this test https://github.com/Level/leveldown/blob/master/test/iterator-test.js#L138-L151 is really testing AbstractIterator and not really implementations, right?

vweevers commented 6 years ago

See the list in https://github.com/Level/abstract-leveldown/issues/269 (every item can be moved here except for the striked-through)

I prefer to solve the serialization stuff before we move tests though, so that we can first see that existing tests do not break.

ralphtheninja commented 6 years ago

I prefer to solve the serialization stuff before we move tests though, so that we can first see that existing tests do not break.

I'll hold off with abstract-leveldown and pick something else. There's plenty to do :wink: