accept level instances and don't create them

[sergioramos]

I have a fork (https://github.com/ramitos/level-modella) of this repo with the following changes:

• accept level instances and don't create them
• when Model.get is called, the callback value is an instantiated Model

This allows to have a sublevel(https://github.com/dominictarr/level-sublevel / https://github.com/stagas/sublevel) for each Model.

I also added some tests (100% coverage).

Do you want me to do a PR, or should I keep my repo as an independent fork?

[rschmukler]

@matthewmueller maintains the leveldb adapter but I vote yes for the merge, especially on .get stuff.

[matthewmueller]

Sweet. So my thoughts:

accept level instances and don't create them

Why don't we support both? If you pass a string in create, otherwise do not.

when Model.get is called, the callback value is an instantiated Model

This is a bug due to modella changes. We should also be returning an array of modella instances when we call Model.all.

[sergioramos]

I also changed the name to keep the naming convention in the level community

[rschmukler]

FYI Modella supports array instantiating... var instances = new Model(instances);

[sergioramos]

Why don't we support both? If you pass a string in create, otherwise do not.

I don't think it should be the adapter responsibility to instantiate the db. But it's a personal "taste", and I would be willing to implement that if you really think it's the best API.

This was actually due to how we changed modella. We should also be returning an array of modella instances when we call Model.all.

I didn't forget to implement .all. I don't think it should be implemented. If a user wants to get all the models it should do it on it's own risk.

If .all would be implemented, it should be just a shim to createValueStream with an ordered-through. Prototype:

var through = require('ordered-through'),

level_modella.all = function(opts) {
  var self = this

  return this.db.createValueStream(opts).pipe(through(function (value, fn) {
    fn(null, self(value))
  ))
}

Returning an array with all the models would be crazy: imagine a Model with a million instances :)

[matthewmueller]

I don't think it should be the adapter responsibility to instantiate the db.

I think in most cases you're right. but it's simple to add and it makes bootstrapping a bit simpler.

I didn't forget to implement .all. I don't think it should be implemented. If a user wants to get all the models it should do it on it's own risk.

Actually haven't been keeping up with leveldb lately, but is there any concept of running a streaming query?

[sergioramos]

Actually haven't been keeping up with leveldb lately, but is there any concept of running a streaming query?

yes:

var stream = db.createReadStream() //createKeyStream, createValueStream and createWriteStream are also available

stream.on('data', function (data) {
  console.log(data.key, '=', data.value)
})

stream.on('error', function (err) {
  console.log('Oh my!', err)
})

stream.on('close', function () {
  console.log('Stream closed')
})

stream.on('end', function () {
  console.log('Stream closed')
})

[matthewmueller]

Oh okay sweet. As a mental TODO, we should probably expose that on the API somehow.

On Dec 2, 2013, at 4:31 PM, Sérgio Ramos notifications@github.com wrote:

Actually haven't been keeping up with leveldb lately, but is there any concept of running a streaming query?

yes:

var stream = db.createReadStream() //createKeyStream, createValueStream and createWriteStream are also available

stream.on('data', function (data) {
console.log(data.key, '=', data.value)
})

stream.on('error', function (err) {
console.log('Oh my!', err)
})

stream.on('close', function () {
console.log('Stream closed')
})

stream.on('end', function () {
console.log('Stream closed')
})
—
Reply to this email directly or view it on GitHub.

[sergioramos]

@matthewmueller @rschmukler pushed the desired changes: https://github.com/ramitos/level-modella/commit/c42de78d5b35972b34c5f0e4c8328ba2fde0c01c