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

Explicit connect / disconnect in level adapter, etc #230

Merged
merged 15 commits into from
Sep 18, 2014
Merged

Explicit connect / disconnect in level adapter, etc #230

merged 15 commits into from
Sep 18, 2014

Conversation

mshick
Copy link
Contributor

@mshick mshick commented Sep 16, 2014

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:

  • Options removes the extra "multilevel" property, and moves those properties up to the main options object. This is because the extra options don't interfere, and it brings better parity with other adapters.
  • If a model is loaded and does not have an id property, I set the id property to the key that was loaded. Model assumes id=key, but in Level the key is actually the required param. I noticed the mongo adapter does something similar, setting the "id" to the true "_id". I found this useful for getting Model to hook into data Model didn't necessarily create.
  • I made the connect method actually do the connecting for multilevel. This allows you to provision the models, and then connect the adapters as a separate action. Again, I'm following the Mongo adapter here.
  • The connect process is now more robust, actually waiting for a connection before emitting an error or a connect, performing auth, and setting the adapter db.
  • The disconnect method now closes the db.

@mshick
Copy link
Contributor Author

mshick commented Sep 16, 2014

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

@mde
Copy link
Contributor

mde commented Sep 16, 2014

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

mshick commented Sep 16, 2014

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 #230 (comment).

@mde
Copy link
Contributor

mde commented Sep 16, 2014

EXCELLENT. :)

@mshick
Copy link
Contributor Author

mshick commented Sep 17, 2014

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

mde commented Sep 17, 2014

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

mshick commented Sep 18, 2014

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.

mshick added a commit that referenced this pull request Sep 18, 2014
Explicit connect / disconnect in level adapter. Multilevel updates, cleanup, and adding tests. Changing dropTable() behavior in Level adapter.
@mshick mshick merged commit d272691 into geddy:master Sep 18, 2014
@mde
Copy link
Contributor

mde commented Sep 19, 2014

EXCELLENT. Thanks so much for this!

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

Successfully merging this pull request may close these issues.

2 participants