Skip to content
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

accept level instances and don't create them #1

Open
sergioramos opened this issue Dec 2, 2013 · 9 comments
Open

accept level instances and don't create them #1

sergioramos opened this issue Dec 2, 2013 · 9 comments

Comments

@sergioramos
Copy link

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
Copy link
Member

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

@matthewmueller
Copy link
Member

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
Copy link
Author

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

@rschmukler
Copy link
Member

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

@sergioramos
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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 [email protected] 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
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants