geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

Explicit connect / disconnect in level adapter, etc #230

Closed mshick closed 9 years ago

mshick commented 9 years ago

Sorry, I ended up with a less clean PR than I would have liked, got a little rushed on the project I was using this for.

Here I'm changing:

mshick commented 9 years ago

One other thing -- the options will now take a manifest object, as well as a path.

mde commented 9 years ago

I've added you to the Geddy org -- you should be getting an invitation shortly. Feel free to merge this PR -- we just need to make sure that the 0.10 tests are passing.

mshick commented 9 years ago

Thanks!

I’ll work out the tests, and get it merged up.

On Sep 15, 2014, at 9:17 PM, Matthew Eernisse notifications@github.com wrote:

I've added you to the Geddy org -- you should be getting an invitation shortly. Feel free to merge this PR -- we just need to make sure that the 0.10 tests are passing.

— Reply to this email directly or view it on GitHub https://github.com/geddy/model/pull/230#issuecomment-55682846.

mde commented 9 years ago

EXCELLENT. :)

mshick commented 9 years ago

I've got in a test for multilevel now and both the original Level adapter test and it pass. I haven't added coverage for sublevel yet, though it should probably make it in there at some point.

Only issue is, I see these Travis tests failing with Riak. Not being familiar with Riak, is that something you'd be able to sort out so we can have a happy machine before I merge in my changes?

mde commented 9 years ago

Riak has been a continual thorn in our sides. I am disabling the those tests in the interest of getting us green. I'll take a look at them when I can. They're not critical.

mshick commented 9 years ago

Finally passing... I had to re-do the dropTable() method in the adapter to use db.batch() rather than a writeStream() because the writeStream seemed to keep the stream open indefinitely, not allowing the db to close without an error.

This might actually be better long-term since LevelUp is threatening to drop the writeStream() method altogether.

mde commented 9 years ago

EXCELLENT. Thanks so much for this!