Closed vweevers closed 2 years ago
@Level/core Side note: I'm concerned about the bus factor here, adding code to an already-refactored code base that no one else is familiar with. Realistically, there's not much I can do about that, besides spending more time not writing code (i.e. looking for contributors, funding, doing that awful thing called self-marketing). I'm still having fun here though, so I'm moving ahead. Just let me know if there's anything I can do to facilitate code reviews.
Hey @vweevers, thanks for all the work you're putting in and the clarity of communication! I personally currently don't have the time to look at large PRs, so if it made sense to make smaller PRs that would help me but also I don't think the bus factor is critical here as everything looks to be well organized and your direction is clear
@juliangruber Noted, thanks! Once I'm past the current refactoring/forking stage, smaller PRs will be easier. For now I will remind myself to make less (unrelated) tweaks, like how this PR includes README changes that I could easily do separately.
Added tests and needed a few more things to make those pass:
nextv()
and all()
to honor the limit
option, abstract iterators now count how many items they yielded, which may remove the need for implementations to do so on their own. Iterators have new public count
and limit
properties. The limit
option now also accepts Infinity
, with the same meaning as -1
(technically that was already the case, but it's now explicit).gte
and lte
range options take precedence over gt
and lt
respectively - on iterators, clear()
and seek()
. This is incompatible with ltgt
but aligns with subleveldown
, level-option-wrap
and half of leveldown
. There was no good choice.To improve coverage, test/self.js
now includes a minimal abstract-level
implementation, which only supports utf8 but is otherwise fully compliant, and is used to test the abstract test suite. Without it, coverage is 91%. With it's 98%. Also useful just to demonstrate the new API's and the problem that I outlined in the OP:
This PR is good to go, but the amount of code makes me want to sleep on it.
I'm happy with the public API. Less so with the private API and tests (which in short are "structurally inconsistent") but it's easy enough (for implementations) to work around. In addition, I want to start benchmarking (https://github.com/Level/abstract-level/issues/4) and I'm making it harder for myself to benchmark fairly when I'm moving further away from abstract-leveldown
and levelup
. So, I'm opening an issue to later refactor the private API (likely in a next major).
I'm squeezing this in for similar reasons as #8. I wondered whether these additions would be breaking. The short answer is no. In addition, adding this now means
level-read-stream
can make use of it without conditional code paths based ondb.supports.*
.Ref Level/abstract-leveldown#380
Adds key and value iterators:
db.keys()
anddb.values()
. As a replacement for levelup'sdb.create{Key,Value}Stream()
. Example:Adds two new methods to all three types of iterators:
nextv()
for getting many items (entries, keys or values) in one call andall()
for getting all (remaining) items. These methods have functional but suboptimal defaults:all()
falls back to repeatedly callingnextv()
and that in turn falls back tonext()
. Example:Adds a lot of new code, with unfortunately some duplicate code because I wanted to avoid mixins and other forms of complexity, which means key and value iterators use classes that are separate from preexisting iterators. For example, a method like
_seek()
must now be implemented on three classes:AbstractIterator
,AbstractKeyIterator
andAbstractValueIterator
. This (small?) problem extends to implementations and their subclasses, if they choose to override key and value iterators to improve performance.On the flip side, the new methods are supported across the board: on sublevels, with encodings, with deferred open, and fully functional. This may demonstrate one of the major benefits of
abstract-level
overabstract-leveldown
paired withlevelup
.Yet todo:
level-concat-iterator
in existing testsAfter that, try another approach with amode
property on iterators, that is one of entries, keys or values (moving logic to if-branches... I already don't like it but it may result in cleaner logic downstream).