Level / abstract-leveldown

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

Consider a factory pattern in test suite #227

Closed vweevers closed 6 years ago

vweevers commented 6 years ago

Where testCommon.factory() would return a fresh db. This allows implementations to add options to their constructor, if they need to. See https://github.com/andrewosh/hyperdown/issues/2 for background.

@andrewosh @ralphtheninja @juliangruber

vweevers commented 6 years ago

@andrewosh here's another example of constructor options: https://github.com/Level/level-js. With levelup, it can be used like so:

var db = levelup(leveljs('my-db', { prefix: 'my-app-' }))

IMO this is much cleaner than open options, which would have to be passed through levelup. It wouldn't be immediately apparent that the prefix option belongs to level-js:

var db = levelup(leveljs('my-db'), { prefix: 'my-app-' })

@ralphtheninja maybe we should start following the constructor pattern everywhere, including leveldown.

ralphtheninja commented 6 years ago

@ralphtheninja maybe we should start following the constructor pattern everywhere, including leveldown.

:+1:

andrewosh commented 6 years ago

@vweevers That's a good point about knowing where the opts come from. I'll update my module to use constructor options. If you and @ralphtheninja want to go ahead with this pattern, do you have thoughts on how the tests will be updated to support this?

vweevers commented 6 years ago

Not yet, there are a lot of angles to consider (and we're currently working on other stuff).

As a temporary solution, you might be able to pass a wrapper to the abstract tests. Along the lines of

require('abstract-leveldown/abstract/get-test').all(wrapper, test)

function wrapper (location) {
  hyperdown.call(this, location, {
    reduce: (a, b) => {
      if (!a) return b
      return a
    }
  }
}

util.inherits(wrapper, hyperdown)
andrewosh commented 6 years ago

Thanks a bunch! I'll give that a shot